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

Normalize and store PB stage times #1181

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

jedso
Copy link
Contributor

@jedso jedso commented Dec 11, 2022

Key changes:

  • Each stage time now has a foreign key referencing an existing playertime ID, so redundant columns that were a part of stagetimeswr such as auth, map and style are now derived from the parent playertimes entry.

  • Deleting a playertime now deletes the associated stage times through CASCADE referential action. Previously, when a WR playertime was deleted it created orphaned child records in stagetimeswr because there was no application logic to delete the stagetimeswr rows.

  • Stage times are now stored for all PBs, not just a WR. Will allow for PB stage splits to be shown during a run – e.g. something like this:
    image

A few minor fixes/changes too:

  • gI_LastStage now resets to 0 when you enter/re-enter the main start zone/bonus start zone if currently on a bonus track. Previously only the restart command reset gI_LastStage.

  • Updated ZoneStageEnter translation (when there are no WR stage times) to match the WRStageTime message translation.

  • Having a stage time of 0.0 no longer shows a ZoneStageEnter/WRStageTime message. This would happen if you have a stage 1 zone intersecting with the start zone.

This PR doesn’t address the implementation of !wrcp, which would need to be a separate table because individual stage time records won’t necessarily be tied to an existing playertime.

The actual migration uses a new stagetimes table with playertimes_id {PK, FK}, stage {PK} and time columns. Technically the stage number column is redundant, and a mapzones id could be referenced to get the stage number from the data column. I opened #1177 to accommodate an implementation that does this, and don’t mind updating the schema/logic if you think it’s a good idea. Would have the benefit of stage times always being associated with the correct stage if a stage zone’s stage number is updated, as well as stage times being deleted at the same time the referenced zone is deleted.

Tested the updated queries in shavit-wr with SQLite and MySQL and both should be working fine. There’s definitely room for cleaner MySQL-specific queries though, as I was focused more on SQLite compatibility. Needs support for Postgres.

Should note that any orphaned records in stagetimeswr are deleted during the migration so you may want to encourage database backups before installation just in case people are relying on WR stage splits that do not belong to an existing playertime.

@rtldg
Copy link
Collaborator

rtldg commented Dec 12, 2022

I'll go through and check this after the latest advent of code

@rtldg
Copy link
Collaborator

rtldg commented Dec 12, 2022

Also, what are your thoughts on using the same kind of "more precise" floats for the stage times, as playertimes?
AKA, what exact_time_int was replaced with:
UPDATE table SET time = %.9f & SELECT %s with gI_Driver == Driver_mysql ? "REPLACE(FORMAT(time, 9), ',', '')" : "printf(\"%.9f\", time)".
It's kind of gross to do but it helps keep the same f32 encoding from sp -> db -> sp

@jedso
Copy link
Contributor Author

jedso commented Dec 14, 2022

Also, what are your thoughts on using the same kind of "more precise" floats for the stage times, as playertimes?

Ah, I forgot about this. Both issues should now be addressed 👍

@Nairdaa
Copy link
Contributor

Nairdaa commented Dec 15, 2022

Will this be merged into the timer?

@rtldg
Copy link
Collaborator

rtldg commented Dec 17, 2022

Will this be merged into the timer?

hopefully later today when I test it

@jedso
Copy link
Contributor Author

jedso commented Dec 17, 2022

Decided to reset last stage to 0 during StartTouchPost instead of being called every tick in TouchPost. Reason being that if you had a start zone intersecting with a stage zone (e.g. stage 1), last stage would never be set to 1 since it gets immediately overridden. Should work fine now for that scenario.

@Nairdaa
Copy link
Contributor

Nairdaa commented Dec 18, 2022

"had a start zone intersecting with a stage zone (e.g. stage 1), "

Isn't startzone stage one by default? As in, if you make a stage mid map and run into it, it should show "Stage: 2", while leaving the startzone should be "Stage: 1". Correct me if i'm wrong.

@rtldg
Copy link
Collaborator

rtldg commented Dec 18, 2022

"had a start zone intersecting with a stage zone (e.g. stage 1), "

Isn't startzone stage one by default? As in, if you make a stage mid map and run into it, it should show "Stage: 2", while leaving the startzone should be "Stage: 1". Correct me if i'm wrong.

IMO it's never really been cemented for stages in bhoptimer since there's only "Stage" zones instead of both "Stage start" & "Stage end" zones.

With how it is, I think treating "Stage" zones as "Stage end" zones is a good way to do it.
AKA you'd be on stage 1 by default and then you'd hook the teleporter to stage 2 with as the stage 1 zone. And then the stage time when you hit the tele would count for the stage 1 time.
Idk if it's the best but it seems okay...

Maybe the expected way to use them should be clarified somewhere or a "Stage start"/"Stage end" zone could be added to make it more explicit.

@jedso
Copy link
Contributor Author

jedso commented Dec 20, 2022

With how it is, I think treating "Stage" zones as "Stage end" zones is a good way to do it.
AKA you'd be on stage 1 by default and then you'd hook the teleporter to stage 2 with as the stage 1 zone. And then the stage time when you hit the tele would count for the stage 1 time.

If !wrcp is going to be implemented in the future, then this isn't a good approach because you need a stage start zone after the tele to know when to start timing an individual stage run time. I took a look at how Momentum Mod and a random surf server handles stages, and it looks like a single stage zone acts as both a "Stage start"/"Stage end" zone simultaneously.

E.g. you would place a "Stage 2" zone after the teleport, and for a full track run touching it would mark the end of how long it took you to complete Stage 1. Jumping/leaving the Stage 2 zone would then begin the individual stage timer, so it also acts as a stage start zone. FWIW, in surf it seems like the individual stage timer only starts if there is no bhop pre-speeding in the stage start zone.

On that point, the other issue to work out is exactly what a stage time is measuring. Currently, IMO, the ZoneStageEnter message You have reached stage N with a time of XX:XX. implies that a stage time is recorded when somebody touches the next stage zone. E.g. as a diagram:
touch-zone-v4

The other way of measuring a stage is the time taken to complete each individual stage. This is obviously what you would want for !wrcp, but it can also apply to a full track run if logic is added to support a single stage zone acting as both a "Stage start"/"Stage end" zone:
each-stage-v1

Rewording ZoneStageEnter from "reached" → "completed" makes more sense in this case.

The second approach is exactly how Momentum Mod handles stage splits and stage zones currently. This includes showing the stage 3 split as a duplicate of the final playertimes time (57.25), which in practice I don't think you'd actually want to store in the stagetimes table. Timing stage 3 start zone → track end zone is really only useful for individual !wrcp stage times, otherwise it's just redundant data.

As it exists right now, the timer can do the first approach without any further changes. The second approach requires additional logic but would also help with a future !wrcp implementation. Let me know any thoughts.

@Nairdaa
Copy link
Contributor

Nairdaa commented Jul 21, 2024

Bump :P

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.

3 participants