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

Rework server stepping and dtime calculation #13370

Merged
merged 2 commits into from Dec 25, 2023

Conversation

Desour
Copy link
Member

@Desour Desour commented Mar 29, 2023

Old:

  • game or dedicated_server_step increment server dtime counter. ServerThread
    runs in a loop in which it receives for at least 30 ms (hardcoded in Server::start()
    via m_con->SetTimeoutMs(30);), and then reads and resets the dtime counter.
    If the counter is >= 1ms, it does a server-step (AsyncRunStep).
  • There are aliasing effects. E.g. in singleplayer the server-step dtimes are
    0ms (0 frames, server-step skipped), 16ms (1 frame) and 33ms (2 frames), and
    the actual times between the server-steps are >30ms or >60ms.

New:

  • ServerThread receives (with timeout) until next step shall start (or longer
    if there are more packets), and calculates dtime itself.
  • In singleplayer, Game tells the server if the game is paused or unfocused.

Effects:

  • Shorter server dtimes are possible (i.e. <30ms).
  • No dtime value aliasing.
  • Server-step dtime is decoupled from client-step dtime (i.e. low client fps
    doesn't cause coarse server-steps).
    Note that pause and unfocus are now handled explicitly. But the results are
    similar to before.
  • The server steps are not slower if no packets are received (fixes issue reported at Fix a very long time bug with server step when no one is moving #14093).
    (In master, the receive of the first packet has a timeout of 30 ms.)

To do

This PR is a Ready for Review.

How to test

  • Try to successfully start singleplyer and dedicated server.
  • Print the dtime in Server::AsyncRunStep to errorstream.

To test the #14093 issue:

  • Set dedicated_server_step to 0.016.
  • /lua local function f() core.sound_play("item_drop_pickup_1") core.after(0, f) end f()
    (Sound from here: https://github.com/minetest-mods/item_drop/blob/master/sounds/item_drop_pickup.1.ogg Other short sounds might work as well.)
  • Alternatively, again print the dtime in Server::AsyncRunStep to errorstream.
  • Notice how in master the sound plays faster (and the dtime is always about 0.016 s) when the player moves, or when you move the camera. With PR, dtime is always 16 ms.

@Desour Desour added @ Server / Client / Env. Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Mar 29, 2023
@Desour Desour force-pushed the simpler_server_stepping branch 2 times, most recently from 2a742d5 to 86c651e Compare March 29, 2023 14:23
@sfan5 sfan5 self-requested a review April 8, 2023 18:21
@Desour
Copy link
Member Author

Desour commented Apr 8, 2023

(Rebased.)

src/network/connection.h Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
src/server.cpp Outdated
// Already break if there's 1ms left, as ReceiveTimeoutMs is too coarse
// and a faster server-step is better than busy waiting.
if (remaining_time_us() < 1000.0f)
break;
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this mean the server step will randomly jitter from 0-999us?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does. And I'd use µs precision here. But to do that, Semaphore::wait would need to be changed accordingly, and win32's WaitForSingleObject only accepts ms.

src/server.cpp Outdated Show resolved Hide resolved
src/server.cpp Show resolved Hide resolved
src/server.cpp Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
src/porting.h Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 changed the title Simplify server stepping and dtime calculation Rework server stepping and dtime calculation Apr 13, 2023
@sfan5 sfan5 added Feature ✨ PRs that add or enhance a feature and removed Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Apr 13, 2023
@Desour
Copy link
Member Author

Desour commented Apr 16, 2023

Rebased.

@Desour Desour requested a review from sfan5 April 27, 2023 14:29
@rubenwardy rubenwardy added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label May 27, 2023
@Desour
Copy link
Member Author

Desour commented Jul 24, 2023

Rebased.
And fixed my misunderstanding of the time delta function (see commit description).

src/client/game.cpp Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
src/server.cpp Outdated Show resolved Hide resolved
@Zughy Zughy added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 2, 2023
@Desour
Copy link
Member Author

Desour commented Oct 2, 2023

Trivially rebased, and comments addressed (last commit). Thx for reviewing.

@Desour Desour removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Oct 2, 2023
@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label Dec 12, 2023
@Zughy
Copy link
Member

Zughy commented Dec 12, 2023

@Desour rebase needed

@Desour
Copy link
Member Author

Desour commented Dec 24, 2023

Rebased. And tested again. Everything works as expected.

@Desour Desour removed the Rebase needed The PR needs to be rebased by its author. label Dec 24, 2023
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

The server and client steps still run. Looks good.
I did not check against the time stop issue when no-one is online but given the timeout code in Server::Receive.

@lhofhansl
Copy link
Contributor

Haven't had a chance to try. Looks right.

@sfan5 sfan5 merged commit 322c4a5 into minetest:master Dec 25, 2023
13 checks passed
@Desour Desour deleted the simpler_server_stepping branch December 25, 2023 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Feature ✨ PRs that add or enhance a feature Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️ @ Server / Client / Env.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants