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: Lunar Subterrane -- Add Forsaken Fount triggers #5926

Merged
merged 3 commits into from
Nov 15, 2023
Merged

raidboss: Lunar Subterrane -- Add Forsaken Fount triggers #5926

merged 3 commits into from
Nov 15, 2023

Conversation

JLGarber
Copy link
Collaborator

The in-line comments should largely cover everything. It's maybe more complex than other possibilities for how we can handle the situation, but it does work.

ui/raidboss/data/06-ew/dungeon/the_lunar_subterrane.ts Outdated Show resolved Hide resolved
const xSum = data.fountX.reduce((a, b) => a + b, 0);

// Don't rely on the rounded sum to be precise, but the sign will be reliable.
if (xSum > 5)
Copy link
Owner

Choose a reason for hiding this comment

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

Can you give an example of what these >5, <5, >10, <10 patterns look like in terms of values? Summing makes sense as a way to differentiate between cases when you know the full set of examples, but it's a bit confusing to a reader. (Also, minorly and mostly curiously rather than something to fix, it makes me wonder if there's some simpler way to differentiate?)

Copy link
Collaborator Author

@JLGarber JLGarber Nov 13, 2023

Choose a reason for hiding this comment

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

Sure, here's a raw list of configurations:

West Safe
(-12.80,-422.00), (0.00,-422.00), (12.80,-422.00)
(9.15,-431.10)
(3.65,-412.90)

North Safe
(0.00,-409.20), (0.00,-422.00), (0.00,-434.80)
(9.05,-412.80)
(-9.05,-418.35)

East Safe
(-12.80,-422.00),  (0.00,-422.00), (12.80,-422.00)
(-9.25,-431.05)
(-3.65,-412.95)

Hourglass
(0.00,-409.20), (0.00,-422.00), (0.00,-434.80)
(-9.05,-418.35)
(9.15,-425.55)

And Y-normalized (for readers not as familiar, remember that positive Y values are south, not north!)

West Safe
(-12.80,0), (0,0), (12.8,0)
(9.15,-9.1)
(3.65,9.1)

North Safe
(0,12.8), (0,0), (0,-12.8)
(9.05,9.2)
(-9.05,3.65)

East Safe
(-12.8,0),  (0,0), (12.8,0)
(-9.25,-9.05)
(-3.65,9.05)

Hourglass
(0,12.8), (0,0), (0,-12.8)
(-9.05,3.65)
(9.15,-3.55)

I'm not completely sure this list is exhaustive, but I'm confident the list of orb locations in the inline comment is.

Copy link
Owner

Choose a reason for hiding this comment

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

That makes sense! Can you add this as a comment somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's try this. I don't like how long it is, but the information is there. I also added some more in-line comments at the points where the output is actually returned.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, I think that's helpful for future debugging, especially the "this probably does not exist" kind of comment.

@quisquous quisquous merged commit d0724d4 into quisquous:main Nov 15, 2023
6 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 15, 2023
…gers (#5926)

The in-line comments should largely cover everything. It's maybe more
complex than other possibilities for how we can handle the situation,
but it does work. d0724d4
github-actions bot pushed a commit that referenced this pull request Nov 15, 2023
…gers (#5926)

The in-line comments should largely cover everything. It's maybe more
complex than other possibilities for how we can handle the situation,
but it does work. d0724d4
@JLGarber JLGarber deleted the lunar-subterrane-add-forsaken-fount branch December 2, 2023 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants