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/oopsy: Add Byakko Unreal #502

Merged
merged 7 commits into from
Nov 19, 2024
Merged

Conversation

JLGarber
Copy link
Collaborator

(Sorry I haven't been around for a while, life has been That Way. I'll get my Sphene normal package in place Soon. This one was some quick work I did because I did Unreal today.)

Tested in-game. No visible issues. I made some small stylistic changes to the Stormblood files to better fit modern conventions, since in 2017 we were a lot more freeform about structure and style.

@github-actions github-actions bot added 💬ko 💬cn raidboss /ui/raidboss module util /util 💬de 💬ja 💬fr oopsy /ui/oopsyraidsy module needs-review Awaiting review labels Nov 16, 2024
@Echoring
Copy link
Collaborator

(I'm not sure how this work, but if the timeline text changes, should the timelineReplace also follows?)

@JLGarber
Copy link
Collaborator Author

(I'm not sure how this work, but if the timeline text changes, should the timelineReplace also follows?)

That's entirely true, thank you for catching that. Since you mentioned it, I looked over the replacements and removed the now-superfluous --leap north-- since a bare --north-- already has a built-in replacement.

Copy link
Collaborator

@xiashtra xiashtra left a comment

Choose a reason for hiding this comment

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

Overall looks ok. I added some suggestions to bring the calls more in line with our current standards, and some severity change suggestions to add some variation so everything isn't at the same level.

ui/raidboss/data/07-dt/trial/byakko-un.ts Show resolved Hide resolved
ui/raidboss/data/07-dt/trial/byakko-un.ts Outdated Show resolved Hide resolved
ui/raidboss/data/07-dt/trial/byakko-un.ts Outdated Show resolved Hide resolved
ui/raidboss/data/07-dt/trial/byakko-un.ts Outdated Show resolved Hide resolved
ui/raidboss/data/07-dt/trial/byakko-un.ts Outdated Show resolved Hide resolved
type: 'HeadMarker',
netRegex: { id: '0004' },
condition: Conditions.targetIsYou(),
alarmText: (_data, _matches, output) => output.text!(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
alarmText: (_data, _matches, output) => output.text!(),
alertText: (_data, _matches, output) => output.text!(),

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one I think is worth leaving as an alarm, since if the puddles are dropped poorly they will cause wipes. I don't feel super strongly about that, but I do think there's some merit to having this be alarmed.

ui/raidboss/data/07-dt/trial/byakko-un.ts Outdated Show resolved Hide resolved
ui/raidboss/data/07-dt/trial/byakko-un.ts Show resolved Hide resolved
@github-actions github-actions bot removed the needs-review Awaiting review label Nov 17, 2024
@github-actions github-actions bot added the needs-review Awaiting review label Nov 17, 2024
@github-actions github-actions bot removed the needs-review Awaiting review label Nov 18, 2024
@github-actions github-actions bot added the needs-review Awaiting review label Nov 18, 2024
Copy link
Collaborator

@xiashtra xiashtra left a comment

Choose a reason for hiding this comment

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

This looks ready for merge to me.

@github-actions github-actions bot removed the needs-review Awaiting review label Nov 18, 2024
@JLGarber JLGarber merged commit ce14c90 into OverlayPlugin:main Nov 19, 2024
12 checks passed
@JLGarber JLGarber deleted the byakko-un branch November 19, 2024 02:51
github-actions bot pushed a commit that referenced this pull request Nov 19, 2024
(Sorry I haven't been around for a while, life has been That Way. I'll
get my Sphene normal package in place Soon. This one was some quick work
I did because I did Unreal today.)

Tested in-game. No visible issues. I made some small stylistic changes
to the Stormblood files to better fit modern conventions, since in 2017
we were a lot more freeform about structure and style. ce14c90
github-actions bot pushed a commit that referenced this pull request Nov 19, 2024
(Sorry I haven't been around for a while, life has been That Way. I'll
get my Sphene normal package in place Soon. This one was some quick work
I did because I did Unreal today.)

Tested in-game. No visible issues. I made some small stylistic changes
to the Stormblood files to better fit modern conventions, since in 2017
we were a lot more freeform about structure and style. ce14c90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬cn 💬de 💬fr 💬ja 💬ko oopsy /ui/oopsyraidsy module raidboss /ui/raidboss module util /util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants