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: add forcejump timeline command #5577

Merged
merged 2 commits into from
Jul 10, 2023
Merged

raidboss: add forcejump timeline command #5577

merged 2 commits into from
Jul 10, 2023

Conversation

quisquous
Copy link
Owner

Rather than adding a loop, add a variant of jump called forcejump to the timeline language. If the sync does not occur, the jump will always happen when the time passes. This allows the timeline controller to automatically add loop lookahead rather than manually specifing it.

There's some manual testing of this in the Frankenstein manual test.txt test case.

To track this, nextEvent now has a whole bunch more state to track where it is in the lookahead so that when adding additional events as timer bars, it doesn't duplicate its efforts.

Additionally, since the same event could be added twice (or more) times on the screen, they need to have different ids (now strings instead of numbers, that include the "jump count" of the number of times we've gone through a lookahead jump to differentiate them from their pre-jump selves). Similarly, they also need different sort keys, although this is kind of a hack for now.

This also adds some grumpy documentation of a bunch of issues found in the code and potential followups.
Sorry that this code is a mess, but I didn't want to rewrite the world at this time.

Closes #5144.

@quisquous
Copy link
Owner Author

I'll leave this up for another week or so. If anybody has any opinions here on either top-level direction or implementation details (@JLGarber? @valarnin?), please feel free to chime in (or ask for more time if you want it too!)

I ended up going with forcejump rather than loop for a couple of reasons.

One is just naming: sometimes the jump is forwards (such as when we have a branch into A and B and then A and B come back together again into a common timeline), and so it's not always a loop jumping backwards (where eventually you'd hit the loop / forcejump again). It felt weird to call the former a loop.

The other reason is that while implementing the lookahead (and also in discussion on #5144), I realized that it got confusing to try to think about what happens if the time got past the loop point without syncing. I could imagine going forward in time in some virtual timeline space where we map real time back onto the loop itself with modulo math, but then ... how do you know which syncs apply where? I decided it was easier to just forcejump and say "if you ever get to this point, the timeline will just adjust its real time back to where you're going, and any syncs there will still apply".

I think the only downside to this is that if you have a forcejump window 20,20 or something, it's really always forcejump window 20,0 as the forward looking part of the sync window will never apply. I can't think of any examples where we have a loop, but the loop itself is inconsistent. We do sometimes add big windows, but that's usually as a "just in case" and less a "this has high variability". However, if we wanted to add such "just in case" windows now, we'd need to do it at both the source and destination of the forcejump rather than just in one place. (We could add timeline linting so force users to have to write window 20,0 for forcejump so that it's clear in the timeline as well? Maybe that's obnoxious.)

I guess one final alternative here is that we could add a fake sync in at the jump destination as well such that after you forcejumped, if you see a belated sync from the forcejump point, it would adjust your timing. This is ... a bit weird to think about code-wise, but I could do it.

@valarnin
Copy link
Collaborator

I guess my two points are:

  1. Is this necessary before we end up reworking the format and therefore the underlying classes? Seems like potential breakage mid-raid-tier that we'll end up throwing out anyways later isn't really great? I'd like to get transitioned to a new timeline format before 7.0...
  2. We do sometimes add big windows, but that's usually as a "just in case" and less a "this has high variability".
    • There is variance if bosses are being dragged, especially in older content but even in new trials and stuff there's still some variance.

@quisquous
Copy link
Owner Author

quisquous commented Jun 19, 2023

Is this necessary before we end up reworking the format and therefore the underlying classes?

Uh, when and who is reworking this format? I haven't seen any momentum or discussion on this front, and personally haven't heard enough pain points to understand the motivation here.

EDIT: Sorry for the surprise on my part, but I was caught off guard with this sentiment.

The only discussion on my radar is this discord discussion: https://discord.com/channels/551474815727304704/594899820976668673/1032391897056022581. It included some discussion that locrian working on a new timeline format for lemegeton (which I asked to see, but didn't get shared; I think I saw it it once elsewhere and if that's what it is it's very XML/id heavy and hard to read; locrian would probably say "you're not supposed to read the raw format" but I think we continue to differ on the value of that). I didn't come away from that discussion thinking that "somebody will make a new timeline format and convert all timelines and tooling before 7.0".

In that same discord conversation you say "The timeline as it exists now is very, very fixed and inflexible. It would probably benefit from a significant rewrite", but it's not clear to me what your pain points are such that there would be benefits from a rewrite. (My complaints about the timeline code are largely about the the timeline controller and not the timeline format or parser.) What's on your mind, specifically?

Here are various issues I know about with timelines (in order of my own priority?):

  • making loops is tedious, and tedious to review (this issue)
  • jumps to numbers is fragile (we could probably add named labels)
  • rather than commenting out syncs, it'd be nice to add testsync that test_timeline verifies syncs at the right time but doesn't sync or adjust the time at runtime
  • (inserting some space here between the above things that I think are higher priority and the below things which I think are even lower priority / could sit forever on the backburner)
  • vague desire about labelling raidwides / tankbusters / magical vs physical vs not damage in Proposal: Adding Ways to Differentiate Timeline Mechanics #516 (I don't think we'd add this per line, because that'd get redundant, but could probably add some separate section mapping text => tags)
  • there's no state other than "what is the current time" and so there's no way to say "Ah it was A/B earlier so now this later one has to be B" without a split (however,I feel like triggers largely cover this, and even if this is a "nice to have" it's also a lot of complication and doesn't merit an entirely new timeline format, to me at least)
  • maybe it'd be nice to have some overview "what phase am I in" header line like we discussed in discord (I think this is easily solvable within the bounds of the existing timeline structure)
  • some timelines are hard to start correctly (e.g. castrum in bozja, because you can see multiple timelines at once, but this also barely comes up)
  • no way to handle parallel timelines (e.g. a8s robot phase with different phasing on different robots; arguably this just isn't an issue in nearly any encounters, and so I wouldn't design anything around this problem until is showed up again)

Things I don't think we should change:

  • calling things "raidwide" vs "Gluttony's Augur" (I think there's too many aoes and tankbusters that are different to just call them that; having real names helps [me] associate the in game ability with the action, it makes translations automatic, and I think some sort of icon/color tagging is a better solution here)

I feel a little bit "bought in" to the current timeline format. We've got make_timeline tools for both network log files and fflogs. We've got test_timeline. We've got syntax highlighting. We've got ~240 existing timelines. Mostly my feeling is that if there's truly a need for a new timeline format, I'd like to understand what the needs are, why the current timeline format can't satisfy it, and what the plan is for converting the old timelines and the tooling to the new format.

As for forcejump, my feeling is that any hypothetical new timeline format will need some (automatic) conversion from the old format, and will almost certainly support loops and this sort of thing, and so hypothetically I'll just assume that we could add some conversion for forcejump as well. If anything, it adds more information to the timeline than what is already there.

Rather than adding a `loop`, add a variant of `jump` called
`forcejump` to the timeline language.  If the sync does not
occur, the jump will always happen when the time passes.
This allows the timeline controller to automatically add
loop lookahead rather than manually specifing it.

There's some manual testing of this in the Frankenstein
manual `test.txt` test case.

To track this, `nextEvent` now has a whole bunch more state
to track where it is in the lookahead so that when adding
additional events as timer bars, it doesn't duplicate
its efforts.

Additionally, since the same event could be added twice
(or more) times on the screen, they need to have different
ids (now strings instead of numbers, that include the
"jump count" of the number of times we've gone through a
lookahead jump to differentiate them from their pre-jump selves).
Similarly, they also need different sort keys, although
this is kind of a hack for now.

This also adds some grumpy documentation of a bunch of
issues found in the code and potential followups.
Sorry that this code is a mess, but I didn't want to
rewrite the world at this time.

Closes #5144.
@quisquous quisquous force-pushed the timeline_forcejump branch from 209fb56 to 9517cf7 Compare June 25, 2023 22:45
@quisquous
Copy link
Owner Author

I think the only downside to this is that if you have a forcejump window 20,20 or something, it's really always forcejump window 20,0 as the forward looking part of the sync window will never apply. I can't think of any examples where we have a loop, but the loop itself is inconsistent. We do sometimes add big windows, but that's usually as a "just in case" and less a "this has high variability". However, if we wanted to add such "just in case" windows now, we'd need to do it at both the source and destination of the forcejump rather than just in one place. (We could add timeline linting so force users to have to write window 20,0 for forcejump so that it's clear in the timeline as well? Maybe that's obnoxious.)

I guess one final alternative here is that we could add a fake sync in at the jump destination as well such that after you forcejumped, if you see a belated sync from the forcejump point, it would adjust your timing. This is ... a bit weird to think about code-wise, but I could do it.

I went ahead and did this. There is now an extra synthetic sync after force jumping that contains any "overhang window" time. If there's ever any sync or jump (or the window expires naturally), the synthetic sync will be removed.

@valarnin
Copy link
Collaborator

I think my biggest issues with the current format (aside from what you already mentioned) can be summarized as:

  1. There's no way to handle logic as part of the timeline. Something like GolbezEx would benefit greatly from some sort of logic control due to the way the stored mechanics work, similarly with e12s's stored mechanics
  2. There's no way for a trigger to interact with the timeline (be that a normal triggers trigger, or a timelineTriggers trigger).
    • By interact, I mean something like:
    • check some conditions, and jump to this time
    • alter the text of this timeline entry based on some condition
    • hide this timeline entry based on some condition
    • adjust the duration of this timer bar based on some condition
  3. There's no good way to reference a specific timeline entry. e.g. let's say I want to change the p9s 272.2 "Gluttony's Augur" entry to be something like 272.2 "Gluttony's Augur (heal check)", there's no way to target that specific timeline entry and override it without also clobbering any other entry with the same text of Gluttony's Augur
  4. The timeline format really doesn't handle multiple different encounters in the same zone very well. e.g:
    • the variant dungeon conversation that was part of the discord conversation you linked
    • something like Delubrum Reginae where parties are handling different sets of mechanics
    • The Copied Factory's Hobbes fight

As for discussion about a new format, I remember some other conversation in Discord where some new example formats were thrown about, something about a yml type format and something else that I can't recall off the top of my head. Discord search is just as bad as always though and I can't find them.

@xiashtra
Copy link
Contributor

  • jumps to numbers is fragile (we could probably add named labels)

I haven't thought too much about what would be nice to have in the timeline format, but being able to jump to a label instead of a time would definitely be on the list. It's very easy to modify a timeline and forget to adjust the jumps currently.

It also feels kind of hacky right now the way encounters in the same zone have to be separated by arbitrarily increasing the times of the subsequent encounters. If there was a way to say, label an encounter block and then jump to that block at the appropriate time, instead of increasing the later encounters by +1000, +2000, etc., like we currently do. Bozjan Southern Front and Zadnor are good examples of where it would be really useful.

@quisquous
Copy link
Owner Author

I split off an issue in #5619 to discuss further improvements rather than it being trapped in a PR that will be harder to find in the future.

In terms of this issue, my feeling is that the future falls into one of these buckets:

  • if nothing is changed about timelines, this is a positive step
  • if we incrementally change timelines, this is still a positive step
  • if we fundamentally and completely change timelines, my argument is that any new timeline format will either:
    • have a way to convert from old to new format (this seems important to me, but I guess it's not fundamentally required), and if so I think having a "this jump is a loop with lookahead" is important meta-information about timelines that would help with conversion and at worst could just be treated as a jump
    • no way to convert and we just have two timeline formats side by side, in which case maybe this is slightly wasted work but it's already done and I suspect that we'll easily get through all of the remaining 7.0 things before any "completely changed timeline" work occurs given that the discussion is at the "what do we even need" stage

@quisquous
Copy link
Owner Author

Hey y'all, this has kinda been sitting for ~2.5w without much feedback. Not that there's true urgency, but there were other timeline things I was interested in doing and felt like it made sense to take care of this one first.

If any of y'all are busy but want to take a closer look at that, that's fine. Just let me know what your schedule looks like. Otherwise, I'll likely plan on landing this in the next week barring any major objections.

@JLGarber
Copy link
Collaborator

JLGarber commented Jul 8, 2023

Sorry, I don't really have much to offer beyond "this seems to make sense, let's land it and see what breaks". As far as using this to abstract away the whole lookahead process for fixed timelines, I can get behind that.

To be clear, when the comment notes:

This is intended for loops that will always be taken in an encounter.
When this is used, no "lookahead" loop unrolling is needed,
and the timeline will use the forcejump destination to list events in the future,

Therefore, to handle something like Ultima Warrior in Fractal Continuum (hard), we would still have to use the current unrolled lookahead, right?

@quisquous
Copy link
Owner Author

Therefore, to handle something like Ultima Warrior in Fractal Continuum (hard), we would still have to use the current unrolled lookahead, right?

Yeah, this doesn't handle that. I think a potential followup here is to add a maybejump (or some sort of thing) which would do lookahead text but with question marks and no forced jump. I thought about doing that here, but it got more complicated so I left this as-is.

@wexxlee
Copy link
Contributor

wexxlee commented Jul 9, 2023

I'm in the same camp as others -- conceptually it sounds like a useful, well-thought-out addition. It sounds like forcejump will be the default for handling straightforward loops (e.g. normal raids & trials), and we'd continue to use jump for the more nuanced situations, e.g., multiple branches, multiple encounters in the same zone, etc? (Not trying to oversimplify or generalize, just trying to articulate the default use cases.)

@valarnin
Copy link
Collaborator

Sorry about the lack of feedback on my end, I've been sick the past two weeks or so.

I think this is fine to merge as-is as well, and we can continue discussion in the issue you opened for further adjustments to timeline format when I've got the mental capacity for it again.

@quisquous
Copy link
Owner Author

Sorry about the lack of feedback on my end, I've been sick the past two weeks or so.

No worries! Hope you feel better!

@quisquous quisquous merged commit 159fdcd into main Jul 10, 2023
@quisquous quisquous deleted the timeline_forcejump branch July 10, 2023 18:08
github-actions bot pushed a commit that referenced this pull request Jul 10, 2023
Rather than adding a `loop`, add a variant of `jump` called `forcejump`
to the timeline language. If the sync does not occur, the jump will
always happen when the time passes. This allows the timeline controller
to automatically add loop lookahead rather than manually specifing it.

There's some manual testing of this in the Frankenstein manual
`test.txt` test case.

To track this, `nextEvent` now has a whole bunch more state to track
where it is in the lookahead so that when adding additional events as
timer bars, it doesn't duplicate its efforts.

Additionally, since the same event could be added twice (or more) times
on the screen, they need to have different ids (now strings instead of
numbers, that include the "jump count" of the number of times we've gone
through a lookahead jump to differentiate them from their pre-jump
selves). Similarly, they also need different sort keys, although this is
kind of a hack for now.

This also adds some grumpy documentation of a bunch of issues found in
the code and potential followups.
Sorry that this code is a mess, but I didn't want to rewrite the world
at this time.

Closes #5144. 159fdcd
github-actions bot pushed a commit that referenced this pull request Jul 10, 2023
Rather than adding a `loop`, add a variant of `jump` called `forcejump`
to the timeline language. If the sync does not occur, the jump will
always happen when the time passes. This allows the timeline controller
to automatically add loop lookahead rather than manually specifing it.

There's some manual testing of this in the Frankenstein manual
`test.txt` test case.

To track this, `nextEvent` now has a whole bunch more state to track
where it is in the lookahead so that when adding additional events as
timer bars, it doesn't duplicate its efforts.

Additionally, since the same event could be added twice (or more) times
on the screen, they need to have different ids (now strings instead of
numbers, that include the "jump count" of the number of times we've gone
through a lookahead jump to differentiate them from their pre-jump
selves). Similarly, they also need different sort keys, although this is
kind of a hack for now.

This also adds some grumpy documentation of a bunch of issues found in
the code and potential followups.
Sorry that this code is a mess, but I didn't want to rewrite the world
at this time.

Closes #5144. 159fdcd
quisquous added a commit that referenced this pull request Jul 10, 2023
quisquous added a commit that referenced this pull request Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

raidboss: new timeline keyword loop
5 participants