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

Complete rework of the ghost and race recorder #869

Merged
merged 39 commits into from Oct 30, 2017

Conversation

Projects
None yet
5 participants
@axblk
Copy link
Contributor

axblk commented Sep 16, 2017

This PR fixes a lot of issues with the ghost and race recorder, introduces a new ghost file format and implements some pretty useful features.

A quick overview:

  • When you cross the start line multiple times, both (ghost and recorder) will only restart if it is a non-solo server
  • If available, both will use the race timer to recognize the start instead of searching for the start line
  • Fastcap support for both
  • The recorder immediately starts when the Tee spawns, so the preparation steps (which are quite important for speedruns) will be included in the demo. If you do not cross the start-line within 20 seconds, it will stop the demo until you really start.
  • Better ghost menu (colors, reloading, deleting and saving ghosts)
  • The ghosts are more resistant against lags (old ones were sometimes completely useless due to small lags)
  • New ghosts files are significantly smaller
  • Cleanup, bugfixes..

About the new ghost format (version 4/5):
This format is used by Teerace for over a year now. The code for handling the binary files was moved to the engine. It includes an auto updater which creates a backup of all ghosts and converts them to the new format afterwards. The major differences from the format used by DDNet right now (version 2), are the ability to store multiple types of data, the usage of delta-encoding and a more portable header.

For whatever reason, the ghost stores values for every predicted tick, but without tick information, so lost snapshots can make them unusable. The new code uses the original values from the snapshots including ticks but it can also handle the old ones without. Since hardly any server uses the high bandwidth option this practically reduces the file size.

Like the demo recorder the ghost recorder directly stores the data to a file (every 50 snapshots) instead of writing the whole file at the end of the race. Indeed this can be changed with only a few lines if the old behavior is preferred.

The updater can handle version 2 (DDNet) and 3 (old teerace format, only slightly different from version 2) files. The updating already happens when the files are scanned for generating the list in the menu and not only when you activate them. The change from version 4 to 5 was only needed due to a bug in the implementation, the ghost loader can read both.

Some numbers about the file size: (map: hotrun, both about 30 seconds)

  • Old ghost: 30.4 kB (converted: 10.7 kB)
  • New ghost: 5.4 kB

One thing about the race recorder:
The old implementation compared the new file only with the first file it found for the particular map. The new one compares with all related demos and deletes them possibly, so that only the best demo is left. Since DDNet can also store the demos without name, this might also delete demos from other players, that you might have in your directory.
To prevent this I at least check whether the demo contains the player name if cl_demo_name is on.
In my opinion the better solution would be to remove cl_demo_name and always use the player name.

@axblk axblk force-pushed the axblk:pr_new_ghost branch Sep 17, 2017

@def-

This comment has been minimized.

Copy link
Member

def- commented Sep 25, 2017

So we get a new ghost format here and a new demo format from heinrich?

To me this change sounds great. Anyone up for a review?

@heinrich5991

This comment has been minimized.

Copy link
Contributor

heinrich5991 commented Sep 25, 2017

I hope to find some time for this.

@Learath2

This comment has been minimized.

Copy link
Contributor

Learath2 commented Sep 27, 2017

  • The better solution would not be to delete anything that we didn't create ourselves we already have that information in the demos, so we should check that. Maybe we should not keep this information in demo names after all, feels better suited to be in the headers.Could give the demo headers an update while we are at it.

  • If you stay on the start line the ghost starts without you and resyncs after a tick, would look better if the ghost was "paused" while you are on the start line.

  • Do we want the record 20s after spawn behaviour on non-race maps as well?

  • /timer command confuses the recorder, might want to get a new NETMSG for sending some kind of race state to properly detect race starts/finishes

@Learath2
Copy link
Contributor

Learath2 left a comment

Just style issues now, will give it another look tomorrow.

src/engine/shared/ghost.cpp Outdated
@@ -0,0 +1,521 @@

This comment has been minimized.

@Learath2

Learath2 Sep 27, 2017

Contributor

?

This comment has been minimized.

@Learath2

Learath2 Sep 27, 2017

Contributor

Should also clean up the file from trailing whitespaces

This comment has been minimized.

@axblk

axblk Sep 27, 2017

Contributor

oops...

src/engine/client/client.h Outdated
const char *GetCurrentMap();
const char *GetCurrentMapPath();

void RaceRecord_GetName(char *pBuf, int Size, int Time = -1);

This comment has been minimized.

@Learath2

Learath2 Sep 27, 2017

Contributor

Don't think we do underscores in function names but should check with @heinrich5991 on that :)

This comment has been minimized.

@axblk
@axblk

This comment has been minimized.

Copy link
Contributor

axblk commented Sep 27, 2017

  1. In general I agree that storing the data in the filename is not a good solution. But storing it in the demo would also introduce some problems. Each file has to be opened (needs some time when you have a lot of demos) and in my opinion the demo header should not be changed for that purpose. The time is a ddnet/race specific thing and demos are for recording gameplay in general. Scanning the whole file for specific NETMSGs would be too slow. The easiest solution would be to move the race demos to a new directory, so no other demos will be deleted.

  2. This should already work. The code might look a bit messy since it has to deal with local and server-side starting behavior (through race timer), but the ghost rendering should only start when the race has not been restarted since the last tick.

  3. I don't see the problem with (up to) 20 additional seconds but it can be limited to non-solo servers easily.

  4. As long as you do not change the timer during your race, the code should be able to handle it. But in general I would prefer to always send the race time (in the warmup value) and add a client-side option for the display type (not visible, replace gametimer and maybe a separate one located between the broadcast and gametimer). Non ddnet clients can still use the chat command and choose between none and broadcast timer. This should be enough to detect the start but adding a finish NETMSG would be good to not rely on the chat parsing anymore (although it should be kept for backward compatibility).

@Learath2

This comment has been minimized.

Copy link
Contributor

Learath2 commented Sep 27, 2017

2 Doesn't work if I compile this PR
4 Guess we could make it work but would need a way to disable /timer, server-side wouldn't work as it'd keep the broken behaviour with old servers and catching the command client-side is sth we don't do iirc. TBH I don't really see how to fix this one for all cases.

@axblk

This comment has been minimized.

Copy link
Contributor

axblk commented Sep 27, 2017

2 I can't reproduce this. When restarting is on, the ghost starts immediately. When it is off, for me the ghost starts when the center of the tee does not touch the start line anymore.

4 Disabling the timer during a race should not be a problem right now. For activating I could add a check if gametick and race start-tick match (or are at least close to each other). When the gametick is significantly higher than the start-tick (maybe one second), the timer has been activated and the ghost won't restart.

@axblk axblk force-pushed the axblk:pr_new_ghost branch to 2586c00 Sep 28, 2017

@axblk

This comment has been minimized.

Copy link
Contributor

axblk commented Sep 28, 2017

Short update:
/timer should not affect the ghost and recorder anymore.
Race demos are stored in demos/auto/race now.

@Learath2

This comment has been minimized.

Copy link
Contributor

Learath2 commented Sep 29, 2017

Interesting that you cannot reproduce 2. I just build it and join a map where I have a ghost on, sit on the start line and watch my ghost go race.

@axblk

This comment has been minimized.

Copy link
Contributor

axblk commented Sep 29, 2017

Just to make sure that I understand you correctly...
You are on a map with restarting (non-solo)
Your tee is inside the start line
The ghost starts but is reset every tick

Timer was on or off?
Official DDNet Server?

@Learath2

This comment has been minimized.

Copy link
Contributor

Learath2 commented Oct 3, 2017

Non-solo map, tee is inside the start line, as soon as I leave the start line the ghost weirdly teleports back and goes where it's supposed to be. I think until you leave the start line the ghost should be paused at the 0 mark.

Timer on, official server.

@axblk

This comment has been minimized.

Copy link
Contributor

axblk commented Oct 6, 2017

I still cannot reproduce it...
Are all maps affected or only a few?
Does it always happen, only sometimes or only one time after you entered the server?

The problem is, that until we receive the snapshot with the race-timer flag set, the client does not know whether the timer is active or not. So until this snapshot arrives, the client has to check for the start itself, based on predicted values. The transition between the local and the server-side starting behavior might cause some problems.
I changed some things in the corresponding code. Maybe one of this changes fixed the issue. Would be good if you could try again with the latest code.

When the problem only occurs one time after you joined a map, the client cannot identify the server as non-solo. But I could not imagine why this would fail. It is directly linked to the team visualization in the scoreboard. Someone would probably have noticed if this feature was buggy.

@Learath2

This comment has been minimized.

Copy link
Contributor

Learath2 commented Oct 16, 2017

All maps affected, It behaves a little different now but still wrong.

Now when you go in the start line the first time it doesn't start as expected, but leave the start line and go back on it and the ghost won't disappear. Causing it to re-sync onto you when you leave the line again.

@axblk

This comment has been minimized.

Copy link
Contributor

axblk commented Oct 18, 2017

Actually this was needed to have a smooth transition from local to server-side ghost control. But I found a solution, so that the ghost is now disabled while you touch the start line.

@def-

This comment has been minimized.

Copy link
Member

def- commented Oct 20, 2017

So, ready to merge? Sounds like something that would be a good addition for DDNet 11.

@Learath2

This comment has been minimized.

Copy link
Contributor

Learath2 commented Oct 22, 2017

Behaviour is fine now \o/ I'll give the code one last look and merge in an hour or so.

@Learath2
Copy link
Contributor

Learath2 left a comment

Can you join Discord or IRC and answer some questions? I really can't seem to understand some of this.

{
return m_DemoRecorder[RECORDER_RACE].IsRecording();
}

void CClient::GhostRecorder_Start(const char *pFilename, const char *pPlayerName)
{
m_GhostRecorder.Start(Storage(), m_pConsole, pFilename, m_aCurrentMap, m_pMap->Crc(), pPlayerName);

This comment has been minimized.

@Learath2

Learath2 Oct 22, 2017

Contributor

IGhostRecorder and IGhostLoader are kernel interfaces, they can request IStorage and IConsole from the kernel.

{
return m_DemoRecorder[RECORDER_RACE].IsRecording();
}

void CClient::GhostRecorder_Start(const char *pFilename, const char *pPlayerName)

This comment has been minimized.

@Learath2

Learath2 Oct 22, 2017

Contributor

If IGhostLoader and IGhostRecorder are kernel interfaces, what is the point of having these functions?

@axblk

This comment has been minimized.

Copy link
Contributor

axblk commented Oct 28, 2017

The things we talked about are fixed now.
I also tried to make OnNewSnapshot at least a bit more readable.
A reliable race timer would simplify the function a bit.

@Learath2

This comment has been minimized.

Copy link
Contributor

Learath2 commented Oct 30, 2017

Thanks :)
bors r+

bors bot added a commit that referenced this pull request Oct 30, 2017

Merge #869
869: Complete rework of the ghost and race recorder r=Learath2 a=Redix

This PR fixes a lot of issues with the ghost and race recorder, introduces a new ghost file format and implements some pretty useful features.

**A quick overview:**
 - When you cross the start line multiple times, both (ghost and recorder) will only restart if it is a non-solo server
 - If available, both will use the race timer to recognize the start instead of searching for the start line
 - Fastcap support for both
 - The recorder immediately starts when the Tee spawns, so the preparation steps (which are quite important for speedruns) will be included in the demo. If you do not cross the start-line within 20 seconds, it will stop the demo until you really start.
 - Better ghost menu (colors, reloading, deleting and saving ghosts)
 - The ghosts are more resistant against lags (old ones were sometimes completely useless due to small lags)
 - New ghosts files are significantly smaller
 - Cleanup, bugfixes..


**About the new ghost format (version 4/5):**
This format is used by Teerace for over a year now. The code for handling the binary files was moved to the engine. It includes an auto updater which creates a backup of all ghosts and converts them to the new format afterwards. The major differences from the format used by DDNet right now (version 2), are the ability to store multiple types of data, the usage of delta-encoding and a more portable header.

For whatever reason, the ghost stores values for every predicted tick, but without tick information, so lost snapshots can make them unusable. The new code uses the original values from the snapshots including ticks but it can also handle the old ones without. Since hardly any server uses the high bandwidth option this practically reduces the file size.

Like the demo recorder the ghost recorder directly stores the data to a file (every 50 snapshots) instead of writing the whole file at the end of the race. Indeed this can be changed with only a few lines if the old behavior is preferred.

The updater can handle version 2 (DDNet) and 3 (old teerace format, only slightly different from version 2) files. The updating already happens when the files are scanned for generating the list in the menu and not only when you activate them. The change from version 4 to 5 was only needed due to a bug in the implementation, the ghost loader can read both.

Some numbers about the file size: (map: hotrun, both about 30 seconds)

 - Old ghost: 30.4 kB (converted: 10.7 kB)
 - New ghost: 5.4 kB


**One thing about the race recorder:**
The old implementation compared the new file only with the first file it found for the particular map. The new one compares with all related demos and deletes them possibly, so that only the best demo is left. Since DDNet can also store the demos without name, this might also delete demos from other players, that you might have in your directory.
To prevent this I at least check whether the demo contains the player name if `cl_demo_name` is on. 
In my opinion the better solution would be to remove `cl_demo_name` and always use the player name.
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Oct 30, 2017

@bors bors bot merged commit de1c0cf into ddnet:master Oct 30, 2017

4 of 5 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
bors Build succeeded
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Jupeyy

This comment has been minimized.

Copy link
Contributor

Jupeyy commented on src/game/client/components/players.cpp in d09e825 Dec 10, 2017

this !Local

@Jupeyy

This comment has been minimized.

Copy link
Contributor

Jupeyy commented on src/game/client/components/players.cpp in d09e825 Dec 10, 2017

and this !Local

This comment has been minimized.

Copy link
Contributor

Jupeyy replied Jan 14, 2018

@def- this is missing?!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment