search

ArchipelagoMW / Archipelago

Archipelago Multi-Game Randomizer and Server
https://archipelago.gg
Other
431 stars 565 forks source link

Aquaria: Renaming some locations for consistency #3533

Closed tioui closed 2 weeks ago

tioui commented 1 month ago

What is this fixing or adding?

Some location names did was not consistent with all other location names. So those locations have been renamed.

There is also a new option, but it is useful only to the client. It does not impact the logic.

Also, I removed the DeathLink option to put it in the client.

How was this tested?

I have generate games with multiple options and have launched the unit tests. Every thing works with no problem.

tioui commented 3 weeks ago

LGTM, it generates, there are not other instances of these particular names in locations, just Cathedral right area being in regions.py, but that's fine

Thanks. At first, I did not understand what you means. Then, I saw that I forgot to rename some Cathedral to Mithalas Cathedral.

Berserker66 commented 2 weeks ago

This also includes a new option, which isn't mentioned in the PR description.

Exempt-Medic commented 2 weeks ago

My signal is bad, but I'm just stating here that I am revoking my approval. Several additional changes were made afterwards

tioui commented 2 weeks ago

This also includes a new option, which isn't mentioned in the PR description.

Oups. Sorry. The new option was add as a subsequent commit. So, I forgot to modify the description. But the option in question does not have any impact on the logic. It is just an information for the client. Also, I removed the DeathLink option after a discussion with @ScipioWright . This option is now activable/desactivable directly from the client.

ScipioWright commented 2 weeks ago

I think their message was more about "the scope of this PR has changed so I would need to re-review it to give approval again". Generally, it's recommended to open a separate PR for separate changes, since that makes it easier to review and avoids situations like this.

tioui commented 2 weeks ago

I think their message was more about "the scope of this PR has changed so I would need to re-review it to give approval again". Generally, it's recommended to open a separate PR for separate changes, since that makes it easier to review and avoids situations like this.

Will know next time.

tioui commented 2 weeks ago

The changes LGTM. Can't test any generations for now, but presumably others did. fill_slot_data could be changed to use options.as_dict and some of the .value's could be removed, but that's outside the scope of this PR.

For the fill_slot_data, I am not certain since I add some information in the slot data that are not in the options (ingredient and dishes randomization). For the .value, I will try removing them for a futur PR.