AndrewSav / lib.remnant2.analyzer

MIT License
3 stars 3 forks source link

Expose worldstone identificators #2

Closed Angelore closed 4 months ago

Angelore commented 4 months ago

Save Worldstone identificators and RespawnLinkNameID. This will allow for displaying the last known character location. We can also add a RespawnZoneID that is right there next to it, but I can't think of a use for it yet.

Save NameId for locations. I think exposing this is pertinent. Getting this information later on is almost impossible. I could not come up with a fitting id for the Walt workaround ("Ancient Canopy/Luminous Vale"), so I put "0000_NotApplicable" there for now.

Small refactor for properties in Zone.cs, with a side effect of also exposing StoryId for those who might want it. It's possible to do a reverse lookup against db file to get it, but I see no reason not to provide it.

AndrewSav commented 4 months ago

Thank you for the PR, description sounds good have not looked at the code yet. A bit unwell at the moment will have a look when I get better.

Angelore commented 4 months ago

Get well soon!

AndrewSav commented 4 months ago

@Angelore, so this is something I also had in my mental to do list, so thank you for picking that up. The question that I have, is why do not we expose RespawnLinkNameID as an actual world stone name. If you look at the RolledWorld properties, you will see that they all are quite high level that can be displayed in the UI. RespawnLinkNameID cannot be displayed there without a lookup, so it's a lower level data. It would be awesome if it called RespawnPoint and contained the world stone name.

I would suggest the following changes:

Small refactor for properties in Zone.cs, with a side effect of also exposing StoryId for those who might want it. It's possible to do a reverse lookup against db file to get it, but I see no reason not to provide it.

The reason I did it the way I did it, is to emphasise that StoryId and Finised are always set once we finished constructing the object graph. One pattern that I implied but not used there is to throw and exception when the Set method are called for the second time. (I know I would not call them twice because the fact that it's a method and not property would be a reminder enough for me not to do that). This being said I do not mind your removing the Set methods, it will work either way and the risk that those values will be overwritten after initial construction (which I wanted to protect from) is low.

The users of the library (R2SA) is not likely interested in ids, if they want to compare ids that it probably should be part of the analyser. I think this property should be removed.

AndrewSav commented 4 months ago

@Angelore I also have this suggestion: why do not we keep public List<string> WorldStones in Location and have a private Dictionary that map WorldStone ids to names (in this case we won't need the WorldStone class either)? Then we can have an internal method GetWorldStoneById or something which will return the string or null. This way interface with R2SA does not have to change at all (not that interface change is a real problem when needed, it's just it seems there is no need for it here). What do you think?

The call can look like this:

string? worldStone = rolledWorld.Zones
  .SelectMany(x => x.Locations)
  .Select(x => x.GetWorldStoneById(respawnLinkNameId)) //respawnLinkNameId comes from the navigator
  .SingleOrDefault(x => x != null);
Angelore commented 4 months ago

@AndrewSav I thought for a long time how to both make the dictionary private and not require calling a method after the object creation to specifically set it (because private fields can not be set via object initialization as was done before). I think adding constructors to Location class is a fair trade off. Let me know if you disagree.

I removed id field from the location. The reason I added it in the first place was to make working with localization easier but then I realized that save files are always going to have location names in English. Because it's a save file, duh.

AndrewSav commented 4 months ago

@Angelore Thank you for this, looking good. I pushed a small change to swap around name and value in the new dictionary and simplify world stone name retrieval. I'm happy to merge this, however...

As far as I now can see there are three different types of player spawn:

Because of this, if the spawn point anything but World Stone we won't be able to detect it. Would you be willing to research a bit more here, and find out if it is possible to retrieve and store link ids for Checkpoint (with the dungeon name I'd presume) and Yellow wall (with to/from location names and side I guess? So we could write in R2SA Respawn Point: Faithless Thicket, Infested Abyss entrance, etc)

Angelore commented 4 months ago

It's funny, all my test saves are at world stones so I didn't think of other cases. Yes, I will look into it. I'm away for about a week, after that I will get back to it.

AndrewSav commented 4 months ago

@Angelore I merged this, since I'm planning to work more on the lib over the weekend, so it's better merge now to avoid merge conflicts. I created an issue here for exposing this in R2SA once we are ready. Please feel free to open a new PR for the checkpoints/transitions. Thank you again, for your help!