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

[Cleanup] Shift-proof "B" Zones #5736

Merged
merged 12 commits into from
May 18, 2024
Merged

Conversation

Frankie-hz
Copy link
Contributor

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Shift proof
Bubu Pennisula
Bostaunieux Oubliette
Bhaflau Thickets
Behemoths Dominion
Beaucedine Glacier & [S]
Bearclaw
Beadeaux [S]
Beadeaux
Batallia Downs [S]
Batallia Downs
Bastok mines
Bastok Markets [S]

Steps to test these changes

Lotto NMs/start server see no lookup errors

scripts/zones/Buburimu_Peninsula/IDs.lua Outdated Show resolved Hide resolved
scripts/zones/Bostaunieux_Oubliette/IDs.lua Outdated Show resolved Hide resolved
SEWER_SYRUP = GetFirstID('Sewer_Syrup'),
ARIOCH = GetFirstID('Arioch'),
DREXERION_THE_CONDEMNED = GetFirstID('Drexerion_the_Condemned'),
PHANDURON_THE_CONDEMNED = GetFirstID('Phanduron_the_Condemned'),
BLOODSUCKER = 17461478,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would something like GetTableOfIDs('Bloodsucker')[X], be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thank you, I meant to put it as a question completely skipped my mind. I wasn’t sure if we wanted a tableid[] or not because there are 15 bloodsuckers I think (on phone cant look).I was brainstorming to see if I could come up with something else that made more sense but I think thats the best idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to [42]. I wish we had an easier way of doing this, it just looks/feels really odd

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if we had something like GetFirstNMId() but I don't think there's any way to differentiate the records at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, its just one dumb list (well, two dumb lists that should be one, but that's a story for another day between NPCs and Mobs)

@claywar
Copy link
Contributor

claywar commented May 17, 2024

CI is definitely not liking something here with the changes, not sure what's going on from the logs yet, but there's definitely an issue vs other branches

@Frankie-hz
Copy link
Contributor Author

I can't seem to narrow is down

@WinterSolstice8
Copy link
Member

Will take a look

@WinterSolstice8
Copy link
Member

Running locally it's fine. hmm...

@WinterSolstice8
Copy link
Member

WinterSolstice8 commented May 17, 2024

I've asked frank to add a commit to allow the workflow to print the exception I suspect it is throwing in Python:

diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml
index 9e37fdf6fe..f2a39fce2e 100644
--- a/.github/workflows/build.yml
+++ b/.github/workflows/build.yml
@@ -509,6 +509,7 @@ jobs:
               hxi_client.logout()
               exit(0)
           except Exception as e:
+              print(e)
               exit(-1)
           EOF
           hxi_result=$?

@WinterSolstice8
Copy link
Member

Well of course it worked now, thanks github

@Frankie-hz Frankie-hz force-pushed the cleanids_16 branch 3 times, most recently from 8079ee8 to 2333c0d Compare May 17, 2024 03:14
@Frankie-hz Frankie-hz force-pushed the cleanids_16 branch 2 times, most recently from de2027e to cf18387 Compare May 17, 2024 03:15
@Xaver-DaRed Xaver-DaRed mentioned this pull request May 17, 2024
4 tasks
@claywar claywar merged commit 52390d2 into LandSandBoat:base May 18, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants