Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raidboss: remove remaining outdated dungeon files #180

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

xiashtra
Copy link
Collaborator

Continuation of quisquous#5964: removes the remaining outdated dungeon files for pre-Trust updated dungeons. Several instances still need to have their triggers/timelines function checked per #150.

@github-actions github-actions bot added raidboss /ui/raidboss module oopsy /ui/oopsyraidsy module needs-review Awaiting review labels Jun 28, 2024
@xiashtra xiashtra enabled auto-merge (squash) June 28, 2024 01:23
@github-actions github-actions bot added the auto-merge PR will be auto-merged upon approving review label Jun 28, 2024
Copy link
Collaborator

@wexxlee wexxlee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The corresponding synthetic ids/zone info in util/zone_overrides.ts should be removed, and gen_zone_id_and_info should be re-run to yoink them out of the resource files. But that script won't run properly right now due to the xivapi schema change to TerritoryType. If you want to wait until #178 is merged, you could add to this PR and do it all at once. Or would you rather land this now and do a follow-up PR after #178 lands?

@github-actions github-actions bot removed the needs-review Awaiting review label Jun 28, 2024
@xiashtra
Copy link
Collaborator Author

xiashtra commented Jun 28, 2024

The corresponding synthetic ids/zone info in util/zone_overrides.ts should be removed

I thought we were keeping the synthetic IDs on purpose like we did with the Unreals just in-case there's references to the old IDs?

But that script won't run properly right now due to the xivapi schema change to TerritoryType

What's the hold-up on their end? The schema has been updated since last night.

Since we're going to have to re-run gen_zone_id_and_info anyways I see no reason not to merge these removals now and have a follow-up that picks up the changes after #171

@wexxlee
Copy link
Collaborator

wexxlee commented Jun 28, 2024

The corresponding synthetic ids/zone info in util/zone_overrides.ts should be removed

I thought we were keeping the synthetic IDs on purpose like we did with the Unreals just in-case there's references to the old IDs?

We keep the synthetic IDs for old unreals because we also keep triggers/timelines for that content (which, maybe we don't need to do, but it's what we've done). So there needs to be a valid zoneId for those files; once we remove timelines/triggers, we no longer need to keep the synthetic zone info. I guess we don't technically need to remove them? But it's basically just clutter at that point.

But that script won't run properly right now due to the xivapi schema change to TerritoryType

What's the hold-up on their end? The schema has been updated since last night.

Schema is updated, but TerritoryIntendedUse went from being a field on TerritoryType to being a relation (TerritoryType.TerritoryIntendedUse.ID), which requires code fixes on our end (already included in #178).

As I'm thinking more about this, we actually still have stale synthetic info for all the reworked dungeons, not just the 6.4 ones, that should probably get cleaned out. Probably worth doing as a separate PR, so I think this is good to land as-is, and we can just add the clean-up to our to-do list (unless you disagree and think we should keep the synthetic stuff around?).

@xiashtra xiashtra merged commit 173d7b3 into OverlayPlugin:main Jun 28, 2024
16 checks passed
@xiashtra
Copy link
Collaborator Author

xiashtra commented Jun 28, 2024

As I'm thinking more about this, we actually still have stale synthetic info for all the reworked dungeons, not just the 6.4 ones, that should probably get cleaned out. Probably worth doing as a separate PR, so I think this is good to land as-is, and we can just add the clean-up to our to-do list (unless you disagree and think we should keep the synthetic stuff around?).

We left the dungeon synthetics when @quisquous merged the last set of removals, but if we really don't have any reason to keep them it would probably be just as well to remove them, though we do have some references in some of the guides to them (for how to set up synthetic zones) that should get cleaned up too if we do that.

@xiashtra xiashtra deleted the remove-outdated-dungeons branch June 28, 2024 02:17
github-actions bot pushed a commit that referenced this pull request Jun 28, 2024
Continuation of quisquous#5964: removes the remaining outdated
dungeon files for pre-Trust updated dungeons. Several instances still
need to have their triggers/timelines function checked per #150. 173d7b3
@wexxlee
Copy link
Collaborator

wexxlee commented Jun 28, 2024

We left the dungeon synthetics when [@]quisquous merged the last set of removals, but if we really don't have any reason to keep them it would probably be just as well to remove them, though we do have some references in some of the guides to them (for how to set up synthetic zones) that should get cleaned up too if we do that.

I know the reworks are done for now, but there might be future scenarios where we do want to keep at least some reference to how to handle synthetic zones. I'll just drop a TODO in the synthetic file, and we can revisit at some point (that's not 6 hours before a new expac 🙃)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge PR will be auto-merged upon approving review oopsy /ui/oopsyraidsy module raidboss /ui/raidboss module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants