Add script to populate notable IDs

[blkerby]

This adds a new property notableId to strats as well as to top-level reusableRoomwideNotable objects.

The fact that reusable notables exist is why there's a need a separate space of IDs for notables, rather than just using strat IDs. It commonly happens that a notable strat later gets changed to be a reusable notable, and we would want to be able to maintain the same notable ID in that case.

Similar to the what #1624 did for tech IDs, having notable IDs will make it possible for Map Rando to preserve notable strat settings in a more robust way that won't get broken by renames. It will also help us with associating videos and difficulty tiers to notables.

This also updates the tests to check that notable IDs (when specified) are unique, or in the case of reusable notables, that they match between the reusable notable strats and their top-level definitions. A check is also added for unused reusable notables; this is currently a warning but could be promoted to an error after the existing instances are fixed.

Note: Some of the existing warnings could be considered false positives since they may be used inside of node unlockStrats rather than inside the room strats array; however, notable strats in unlockStrats are ignored by Map Rando and there is no plan to add randomizer support for them. Instead, now that the strat property setsFlags is available, we should be able to migrate these strats into the room strats array.

[osse101]

1) The reusableRoomwideNotable property on strats should be removed. Because its purpose was to be the ID. 2) Adding notableIDs to all notables doesn't seem required to me. You have stratIDs to connect to and from the shared notableIDs. A notableID being changed to a shared value, and later changed back to unique, is strange in fact. But I'm not thinking through this enough to know what is needed.

[blkerby]

1. The reusableRoomwideNotable property on strats should be removed. Because its purpose was to be the ID.

I was thinking that the notable ID wouldn't be populated by hand. So the reusableRoomwideNotable would still be how the connection gets made. But it is redundant and could be removed if we're ok managing notable IDs manually.

2. Adding notableIDs to all notables doesn't seem required to me.  You have stratIDs to connect to and from the shared notableIDs.  A notableID being changed to a shared value, and later changed back to unique, is strange in fact.  But I'm not thinking through this enough to know what is needed.

So I think if we didn't want to put notable IDs on non-reusable notables, we would just say that the notable ID for a non-reusable notable would be implicitly be assumed to be its strat ID. In that case, when converting a non-reusable notable into a reusable one, we would just populate its notable ID using its strat ID, and then add that same notable ID to any new strats that are part of that same reusable. In general, a reusable notable would have notable ID matching some arbitrary one of the strat IDs associated with it, though it's possible that strat could be deleted at some point and you're left with the notable ID not matching a strat ID anymore, and that can be fine too.

It seems a bit strange in its own way but may be overall simpler I guess. It removes the need for a separate space of IDs for notables, but at the same time it means the notable ID space piggy-backs on the strat ID space in a bit of a tricky way. I'm not sure if it's better or worse than what is proposed in this PR, of assigning notable IDs sequentially as a separate space of IDs. Any thoughts @kjbranch ? Or did you have something different in mind @osse101 ?

[kjbranch]

I think this most recently suggested approach would be a bit better, but still feels messy. Could we instead only add 2 new fields to the reusableRoomwideNotable objects: an ID for it, and an array of all of the strat IDs that share this reusableRoomwideNotable?

[blkerby]

I think this most recently suggested approach would be a bit better, but still feels messy. Could we instead only add 2 new fields to the reusableRoomwideNotable objects: an ID for it, and an array of all of the strat IDs that share this reusableRoomwideNotable?

Interesting, that does sound simpler. I guess in this case the ID for the reusable should still be a strat ID of one of the strats, so that it can remain unchanged when converting a non-reusable notable into a reusable?

[kjbranch]

I was thinking it would be unique and having the strat ID in the list on the reusable would be enough to match it. But it could, if you think that would make things a lot easier.

[blkerby]

So another option which might be even cleaner/simpler is we could define all notables in this section, not only reusable ones. So maybe we rename the "reusableRoomwideNotable" list to just "notableStrats". Then there's less effort to change a notable between non-reusable and reusable: it would simply be a matter of modifying the strat ID list inside the notable, no more need to restructure or move things. And then we could get rid of all the strat properties relating to notables (the "notable" boolean and the "reusableRoomwideNotable" string). It also gives us an option to have a different description (e.g. possibly a shorter one) showing for notables on the Generate page compared to in the individual strat note.

Only thing I'm seeing that could be a bit awkward is that the strat ID would have to be populated before adding a strat to the notable list. So we'd either have to do that manually in the same PR, otherwise it would require merging the first PR, running scripts, and then doing another PR to set up a notable.

On Mon, Sep 9, 2024, 8:37 AM kjbranch @.***> wrote:

I was thinking it would be unique and having the strat ID in the list on the reusable would be enough to match it. But it could, if you think that would make things a lot easier.

— Reply to this email directly, view it on GitHub https://github.com/vg-json-data/sm-json-data/pull/1640#issuecomment-2338302052, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEFQZOTNLRIQTMKAR25DACTZVWXBNAVCNFSM6AAAAABNZIJ6F2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZYGMYDEMBVGI . You are receiving this because you authored the thread.Message ID: @.***>

[osse101]

I've had a view of reusableRoomwideNotables as a tag for presentation and readability. It lets you write descriptions for strat toggles which may not be the same as notes relating to the performance of a strat. In this sense I don't know that I have a preference for notables being declared on the strat or in a Notables section. A notable section sounds pretty cool though.

Manually doing strat IDs to make a notable, or running the script manually, isn't hard to do and coordinate. But its not great having weird steps on a common feature.

[blkerby]

Ok, I took a second pass at it, putting all the notable-related info inside a room-level notables list now. I made changes to the schema and tests but did not include running the script in this PR. So the tests will not pass until after running the script. After this PR is merged, we will want to run the script as an immediate follow-up.

With the new proposed schema, there will be no ongoing need for a script to assign notable IDs (since this can be done manually without much difficulty), so the script is just designed to be run a single time in order to perform the migration, and then we'll be done with it.

A couple other issues with notables that are not in scope of PR but that we'll probably want to address soon:

• Notable strat names usually include the room name but not always in a consistent way. I think we could consider moving away from this, letting the randomizer (or other tools) populate the room name automatically where needed.
• The old notable boolean and reusableRoomwideNotable string are still used in old-style strats inside viewableNodes and unlocksStrats. We should clean these up soon by moving these into regular strats. In the case of viewableNodes we probably need to add a new strat property to indicate item nodes that can be viewed as part of executing a strat.

[blkerby]

What happens when you include a stratID in multiple notables? It depends on each notable being available, or is there some other kind of connection being made?

I hadn't thought about that. But yeah, I think that makes the most sense, that it would require all associated notables to be toggled on in order for the strat to be in logic. I could see that potentially being useful in some rooms, to allow them to overlap like that.

[blkerby]

I had one more idea of a different way of doing this. What if we added a notable logical requirement, kind of similar to the tech logical requirement, referencing a notable by name? So in this scenario, it could look something like this:

{
  "notables": [
    {
       "id": 1,
       "name": "Taco Tank",
       "note": "..."
    }
  ],
  "strats": [
    {
       "id": 1,
       "link": [1, 3],
       "requires": [
          {"notable": "Taco Tank"},
          "canCWJ",
          "canInsaneWalljump",
          "canStationarySpinJump"
       ]
    }
  ]
}

This could have an advantage of being more readable and it would avoid the need to reference strat IDs, which was potentially awkward (since the needed strat IDs might not yet exist) and error-prone (since a typo in a strat ID may not be easy to catch). It is also more flexible, making it possible for only certain branches of an or to depend on a notable requirement, similar to what we can do with tech. The idea is that a notable is essentially just a room-local tech so maybe it should be usable in the same kind of way as a tech.

[osse101]

• The stratIDs array, listing only the IDs, was a good deal less readable. And detached from the strat itself.

• Notables very much are roomwide techs. Techs are notables that occur in multiple rooms, in some cases.

• Not needing to manage the notable strat IDs is a big plus

• I like that multiple notables can be applied to a strat

• OR on a notable could maybe be fine but its more often that it should be a different strat. The note requirement on the strat would go away I suppose, so you would reference the notable note elsewhere somehow. But good practice would have a strat note, and ORs are messy in notes.

So I like the new suggestion more overall.

[kjbranch]

I like this idea as well, but don't really like the idea of putting notables in ORs. However it's fine to keep it as an option and use it seldomly to never.

[blkerby]

Ok, updated it to implement the new approach, hopefully third time is the charm now