search

zeldamods / event-editor

Event flow editor for Breath of the Wild
GNU General Public License v2.0
25 stars 9 forks source link

Add alternate autofill option for games other than Breath of the Wild #8

Closed itsschwer closed 1 year ago

itsschwer commented 1 year ago

The event editor is compatible with Tears of the Kingdom but the the file structure has changed, meaning that the autofill feature is unusable for it. I've tried to add an alternative option that requires more manual work on the user's part but allows them to setup autofill for Tears of the Kingdom (and potentially other games?). The readme has also been updated with a brief usage guide.

Additionally, this is my first time working with Python, so additional changes may be required (will gladly accept any feedback!)

This change is Reviewable

leoetlino commented 1 year ago

IMO the format should be a single file containing action/query definitions for all actors. Actions and queries should also be separate, and it should be clearer what the values are used for. I'd suggest something like this:

{
  "Actor1": {
    "actions": {
      "Action1": {
        "params": {
          "Param1": {"default_value": true},
          "Param2": {"default_value": 42},
        }
      }
    },
    "queries": {} // same format as actions
  },
}

Using JSON for this sounds fine since this kind of file is meant to be automatically generated.

Also, I would call this file an "actor definition file" - "JSON dir" sounds a bit too vague/generic.

itsschwer commented 1 year ago

To clarify the use case, the JSON files are (currently) expected to be user-generated, so that they can add their own parameters for use with autofill. This is why I've opted to use a more minimal/flatter JSON structure, though I do think having a file per action/query is excessive.

As for action and query separation, I'm actually not super familiar with the event flow structure. Is it just that actions belong to Action events and queries belong to Switch events?

leoetlino commented 1 year ago

Yes, and actions/queries can theoretically have the same name so they should be separate.

The reason why I suggested a more complex schema is to make it future-proof (easily extendable) and to make it easier for someone to efficiently auto-generate an EventEditor compatible actor definition file for games like TotK. Creating one file per actor is not super convenient when you have 7000+ actors.

And ideally, we'd also replace the BotW AI specific logic with a separate tool that generates the JSON definitions, so that the JSON file is not just an afterthought/fallback but a flexible mechanism of adding autocomplete (and maybe action/query documentation) - one that works with any game.

itsschwer commented 1 year ago

I've changed the pull request status to draft as I've started to add the export/JSON generation functionality but it's incomplete.

I see your point about using a more complex and descriptive schema for the JSON definitions, but at this stage (and as a user) I wish to continue with a one actor one file approach (so far I haven't encountered a large number of different event actors?) — I'm open to changing this in the future though! Currently the schema looks like this:

// Actor1.json
{
    "actions": {
        "Action1": {
            "Parameter1": true
        }
    },
    "queries": {
        // etc
    }
}

Also, I'm currently a bit stumped on how to populate the parameters with default/sample values, would you be able to provide some direction?

Thanks!

leoetlino commented 1 year ago

I see your point about using a more complex and descriptive schema for the JSON definitions, but at this stage (and as a user) I wish to continue with a one actor one file approach (so far I haven't encountered a large number of different event actors?) — I'm open to changing this in the future though!

I think we should avoid adding a feature only to remove or drastically change how it works later.

In my opinion the UX for the mechanism that's being added in this PR is not great -- it shouldn't be the case that the user has to mess with a JSON file every single time they want to add an action or query to the actor definition set.

Instead, there should be a way to batch export definitions for all actions/queries that are used in an event flow.

Also, I'm currently a bit stumped on how to populate the parameters with default/sample values, would you be able to provide some direction?

This is easy to implement with the batch export idea. Just iterate through the actions and switches in the event flowchart; that way you get both the action/query names and their parameters at the same time.

itsschwer commented 1 year ago

I see where you're coming from about the complexity for general users — I've altered the code to use a single JSON file (but prefer a specific actor file if it exists as an 'override' when loading).

The UX for exporting is still very much a work-in-progress, hence the draft status. The current implementation is just to get something working as I've also been using the tool (and people in the Tears of the Kingdom modding server have found my fork).

The batch exporting is definitely a goal for this pull request, but I'm not experienced enough to know where to start. Would you be able to provide more detail about where would be a good entry point to get the data for iterating?

leoetlino commented 1 year ago

There is an "Export graph data to JSON" feature (for exporting the entire flowchart, not just action/query info) that you can mimic.

https://github.com/zeldamods/event-editor/blob/a30f5db70a7c8ac948c35b1a37638a8768555470/eventeditor/__main__.py#L104

https://github.com/zeldamods/event-editor/blob/a30f5db70a7c8ac948c35b1a37638a8768555470/eventeditor/flowchart_view.py#L193

self.flow_data.flow (in FlowchartView) is the EventFlow object for the currently loaded event flow: https://github.com/zeldamods/evfl/blob/645dc7d786233896561ea27a697e7a171c0828cb/evfl/evfl.py#L10

flow.flowchart.events (https://github.com/zeldamods/evfl/blob/645dc7d786233896561ea27a697e7a171c0828cb/evfl/flowchart.py#L9) is a list of flowchart events that you can iterate on. Take a look at evfl/event.py to see what kind of fields an Event has (I've never written detailed docs for evfl, sorry! but hopefully the source code is easy enough to read)

edit: some additional context. evfl is the Python library that deals with event flow parsing/serialisation. It's what EventEditor (the graphical editor) is built upon.

itsschwer commented 1 year ago

Thanks for the links, they were just what I needed!

I think my implementation is ready for review now, though I'm not sure I've gone about it in a clean way. I'm not sure if this is good practice, but I've left comments to explain my thought process or in areas where I was uncertain — all of this should be in actor_json.py.

To summarise the feature changes:

Thanks again for the guidance!

itsschwer commented 1 year ago

Thanks for the detailed review!

I've tried to address all of the sections you've highlighted, but I'm not completely sure if I've done the type hinting right.

re: linter/mypy I'm not very familiar with the Python workflow, my bad for letting those errors slip through! I'm working in VS Code and have now installed a Mypy extension, but it seems to identify a large number of errors (304) across the project — would you happen to have any experience with this? (all good if not!)

leoetlino commented 1 year ago

I'm not very familiar with the Python workflow, my bad for letting those errors slip through! I'm working in VS Code and have now installed a Mypy extension, but it seems to identify a large number of errors (304) across the project — would you happen to have any experience with this? (all good if not!)

It's been a while since I did any major development work on evfl or event-editor, but iirc PyQt sometimes causes mypy to get really confused. If the errors you're seeing are about PyQt, I wouldn't worry too much about them. We might be able to make them go away by updating our dependencies.