cronza / heartbeatengine Goto Github PK

This project looks really interesting. I think having a bit more tooling around it would make it easier for others to contribute. Some suggestions that you could consider:

Auto-formatter

Having an auto-formatter installed will make the code more uniform and reduce diffs that are just due to formatting.
I personally would recommend using black, as it has (almost) no configuration options, so will behave the same on each machine and avoid discussions about code-formatting.

Tests

Refactoring parts of the project is a bit risky right now, because there are no automated tests. Making a more clear separation between frontend and backend code and covering at least the latter with automated tests would make refactoring easier.
I personally would recommend to use pytest, as it has an intuitive and easy to understand fixture support.

Linter

You also may want to consider setting up pylint to catch issues in the code that may lead to bugs. Especially because there are no tests right now, I feel that'd be helpful and relatively easy to set up.
A further step could be type-checking using mypy, which would come in handy with refactoring like in #23.

In order for the editor to know what the engine is capable of, along with how to represent that as options, it uses a database file called ActionsDatabase.yaml. An example of it's structure:

      action_name: "create_background"
      requirements:
        - name: "key"
          type: "String"
          value: "Background"
          editable: False
          preview: True

        - name: "sprite"
          type: "File_Image"
          value: "None"
          editable: True
          preview: True

        - name: "position"
          type: "Vector2"
          value: [0,0]
          editable: True
          preview: False

It's common in a number of places to parse the requirements key, particularly for the details panel window (Each requirement is added to the list of settings for whatever that editor has selected). However, because it is a list, we always must loop through it in order to access each item (Whether to use that data to generate a new entry in the details panel, or whether to look something up when loading data from a file). In the latter case, it'd be valuable if this was a dict instead so we could look it up by key instead of looping to find it

The best example of this situation is in the database_manager.py file, where we load files that have been saved to disk in the format used by the engine, and convert them into a format used by the editor

1. Open dialogue sequence
2. Add 'Dialogue' action
3. Edit 'Color', and choose any color
4. Edit 'Color again

Example: Double clicking ".wav" file

Currently, input entries can only have static choices based on what was given when they were created. In cases like the old method of doing choices where we wanted a dynamic dropdown of active branches, we resorted to a basic text input to avoid a bunch of custom implementation

Input entries should be able to query something about the editor that owns them (IE. Query the dialogue editor for info on what branches are available)

Items added to the scene view currently allow the user to modify their movement via drag and drop. For items such as the background, the position parameter doesn't appear as it isn't meant to be moved. This makes it difficult to reset the item's position once it's been moved.

To double down on this, it can be generally frustrating as items begin to stack to not accidentally select or move the wrong items. By allowing items to be 'locked', you can allow editing parameters, but force the item to remain still

Fonts with unicode names (Not file names but actual names) are not loading correctly when playing, causing a mismatch with the editor which does support it

Currently, the Settings and Logger classes use a class designed to function as a singleton. This leads to the following in order to use them:
Settings.getInstance().

This can be simplified as modules when imported are functionally singletons. If we remove the class and simply have the functions and variables exist in the module, we can drop the .getInstance() bit

I've always used "index" for variables representing an interator, but I don't feel this is providing any real value, and can be simplified to "i"

Using dialogue_02, changing scenes to the Point&Click scene causes the background and Rei interactable to remain scaled to the smaller resolution

We currently use plain styling, which for multi-line strings leads to some potential conflicts depending on the characters the user inputs (:. ?, etc)

https://stackoverflow.com/questions/3790454/how-do-i-break-a-string-in-yaml-over-multiple-lines/21699210

Certain fonts seem to have different top margins when rendered in the engine. Comfortaa (seen in photo) has no margin when in engine, but has one when in the editor. This is not consistent for all fonts

New projects start with paths referring to engine content files. Once changed, there is no obvious method to navigate back to those files to browse

For objects such as the YAML Reader / Writer, I assumed not providing the self arg made it static. However, I've recently learned of @staticmethod which does enforce it to be static as desired

Currently, editor code is largely split into the core and interface folders, as a way of separating U.I classes from the functional classes. While this originally worked well, some lines are blurring, and the overall structure is dissolving. We should collapse the two into a single structure

Originally, the engine / editor used os.path for all path strings, but after realizing backslashes were enforced on windows (where only forward slashes were needed), this was dropped in favor of manually concatenating. pathlib should provide better support that os.path

"create_choice_button" is unnecessary and can be replaced by creating buttons directly in the "choice" action

Randomly a new drag and drop scene appears to not be a viable file and not created by the editor.

The build call doesn't encapsulate the project path string in quotes, so any spacing is a break in arguments

Apparently PyQt5 supports applying stylesheets to apps, instead of having to manually apply them to each widget individually. Adopting this could massively improve the styling workflow for the editor, including the following benefits:

• Major code reduction and simplification (Less spot changes overall)
• Styles or "Themes" can be consolidated to a single CSS file
• Avoids the custom "EditorStyle.yaml" setup currently in use

• Project creation
• Project loading
• Creating files (Dialogue Scene)
• Adding Dialogue entries
• Adding Dialogue Branches
• Editing Dialogue Entry Details
• Editing Project Settings
• Saving
• Playing Game

When attempting to change branch descriptions, unless you change the branch name, it shows "already in use"

Just skimming over the code base out of curiosity. Noticed your full venv folders were included in the repo (e.g. https://github.com/Cronza/GVNEngine/blob/master/GVNEditor/venv/Scripts/pip.exe). Environments aren't usually included directly in Python repos, but instead their requirements are enumerated either in setup.py (for production-ready packages) or more commonly as a text file (see https://pip.pypa.io/en/stable/user_guide/#requirements-files, or https://github.com/pygame/pygameweb/blob/master/requirements.txt as an example of another project doing just this).

I'd recommend two files replace each of your venv directories:

requirements.core pinned at major versions, written by you. Example:

click==7
pygame==2
PyQt5==5

requirements.txt autogenerated after you pip install -r requirements.core --upgrade; pip freeze -r requirements.txt, pinning specific versions for reproducibility. Example generated from the above requirements.core:

click==7.1.2
pygame==2.0.1
PyQt5==5.15.3
PyQt5-Qt==5.15.2
PyQt5-sip==12.8.1

It's possible to just get away with just requirements.txt, but it's easy to forget what you intended to install, and what got installed on your behalf.

In order to allow U.I elements to impact others without requiring explicit knowledge of their type, I added a function called 'Refresh' to a number of classes. However, this relies on the target having implemented it uniquely, where as we should use an abstract class in order to centralize where that method is coming from