refactor: Server pattern #549
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Great stuff!! |
elementbound
commented
Jan 19, 2026
| # Resolve to identifier | ||
| var identifier := NetworkIdentityServer.resolve_reference(peer, idref) | ||
| if not identifier: | ||
| # TODO: Handle unknown IDs gracefully |
elementbound
commented
Jan 19, 2026
| if buffer == null: | ||
| buffer = StreamPeerBuffer.new() | ||
|
|
||
| # TODO: How about encoding the first snapshot as-is, and then the rest as diffs |
elementbound
commented
Jan 19, 2026
| # Resolve to identifier | ||
| var identifier := NetworkIdentityServer.resolve_reference(peer, idref) | ||
| if not identifier: | ||
| # TODO: Handle unknown IDs gracefully |
albertok
reviewed
Jan 24, 2026
| func record_state(tick: int) -> void: | ||
| var input_snapshot := get_rollback_input_snapshot(tick - 1) | ||
| _record(tick, _rb_state_snapshots, _rb_state_properties, false, func(subject: Node): | ||
| if not subject.is_multiplayer_authority(): |
Contributor
There was a problem hiding this comment.
Server always has authority to broadcast states
Suggested change
| if not subject.is_multiplayer_authority(): | |
| if subject.multiplayer.is_server(): | |
| return true | |
| if not subject.is_multiplayer_authority(): |
Contributor
Author
There was a problem hiding this comment.
Might have to investigate why is_predicting() was returning true in this case. Otherwise we risk overwriting a client's prediction with ticks that the server had no input for.
Contributor
Author
There was a problem hiding this comment.
Without the suggested change, RollbackSimulationServer.is_predicting() would return true for the other player's car on the server, because of this clause:
if is_owned and not has_input:
# We own the node, node depends on input, we don't have data for input - predict
return trueSo as long as we haven't received input from the client, we consider the recorded simulation predicted, which - in theory - makes sense. I guess next thing to find out is why we need to forego the above.
On clients, is_predicting() returns false for the player's car with this clause:
if not is_owned and has_input:
# We don't own the node, but we own input for it - not (input) predicting
return false
albertok
reviewed
Jan 24, 2026
Comment on lines
+387
to
389
| NetworkHistoryServer.restore_rollback_input(tick) | ||
| NetworkHistoryServer.restore_rollback_state(tick) | ||
| on_prepare_tick.emit(tick) |
Contributor
There was a problem hiding this comment.
Signals first
Suggested change
| NetworkHistoryServer.restore_rollback_input(tick) | |
| NetworkHistoryServer.restore_rollback_state(tick) | |
| on_prepare_tick.emit(tick) | |
| on_prepare_tick.emit(tick) | |
| NetworkHistoryServer.restore_rollback_input(tick) | |
| NetworkHistoryServer.restore_rollback_state(tick) |
albertok
reviewed
Jan 24, 2026
Comment on lines
+399
to
400
| RollbackSimulationServer.simulate(NetworkTime.ticktime, tick) | ||
| on_process_tick.emit(tick) |
Contributor
There was a problem hiding this comment.
Suggested change
| RollbackSimulationServer.simulate(NetworkTime.ticktime, tick) | |
| on_process_tick.emit(tick) | |
| on_process_tick.emit(tick) | |
| RollbackSimulationServer.simulate(NetworkTime.ticktime, tick) |
albertok
reviewed
Jan 24, 2026
Comment on lines
+405
to
407
| NetworkHistoryServer.record_state(tick + 1) | ||
| NetworkSynchronizationServer.synchronize_state(tick + 1) | ||
| on_record_tick.emit(tick + 1) |
Contributor
There was a problem hiding this comment.
Suggested change
| NetworkHistoryServer.record_state(tick + 1) | |
| NetworkSynchronizationServer.synchronize_state(tick + 1) | |
| on_record_tick.emit(tick + 1) | |
| on_record_tick.emit(tick + 1) | |
| NetworkHistoryServer.record_state(tick + 1) | |
| NetworkSynchronizationServer.synchronize_state(tick + 1) |
albertok
reviewed
Jan 24, 2026
Comment on lines
+411
to
413
| NetworkHistoryServer.restore_rollback_state(display_tick) | ||
| RollbackSimulationServer.trim_ticks_simulated(history_start) | ||
| after_loop.emit() |
Contributor
There was a problem hiding this comment.
Suggested change
| NetworkHistoryServer.restore_rollback_state(display_tick) | |
| RollbackSimulationServer.trim_ticks_simulated(history_start) | |
| after_loop.emit() | |
| after_loop.emit() | |
| NetworkHistoryServer.restore_rollback_state(display_tick) | |
| RollbackSimulationServer.trim_ticks_simulated(history_start) | |
albertok
reviewed
Jan 24, 2026
Comment on lines
+193
to
+194
| "name": "NetworkSynchronizationServer", | ||
| "path": ROOT + "/servers/rollback-synchronization-server.gd" |
Contributor
There was a problem hiding this comment.
File change fix
Suggested change
| "name": "NetworkSynchronizationServer", | |
| "path": ROOT + "/servers/rollback-synchronization-server.gd" | |
| "name": "NetworkSynchronizationServer", | |
| "path": ROOT + "/servers/network-synchronization-server.gd" |
albertok
reviewed
Jan 24, 2026
Comment on lines
+559
to
560
| NetworkHistoryServer.restore_synchronizer_state(tick) | ||
| before_tick_loop.emit() |
Contributor
There was a problem hiding this comment.
signals before rs's
Suggested change
| NetworkHistoryServer.restore_synchronizer_state(tick) | |
| before_tick_loop.emit() | |
| before_tick_loop.emit() | |
| NetworkHistoryServer.restore_synchronizer_state(tick) |
albertok
reviewed
Jan 24, 2026
Comment on lines
+566
to
568
| NetworkHistoryServer.record_sync_state(tick + 1) | ||
| NetworkSynchronizationServer.synchronize_sync_state(tick + 1) | ||
| after_tick.emit(ticktime, tick) |
Contributor
There was a problem hiding this comment.
here too
Suggested change
| NetworkHistoryServer.record_sync_state(tick + 1) | |
| NetworkSynchronizationServer.synchronize_sync_state(tick + 1) | |
| after_tick.emit(ticktime, tick) | |
| after_tick.emit(ticktime, tick) | |
| NetworkHistoryServer.record_sync_state(tick + 1) | |
| NetworkSynchronizationServer.synchronize_sync_state(tick + 1) |
albertok
reviewed
Jan 24, 2026
Comment on lines
+575
to
576
| NetworkHistoryServer.restore_synchronizer_state(tick) | ||
| after_tick_loop.emit() |
Contributor
There was a problem hiding this comment.
Suggested change
| NetworkHistoryServer.restore_synchronizer_state(tick) | |
| after_tick_loop.emit() | |
| after_tick_loop.emit() | |
| NetworkHistoryServer.restore_synchronizer_state(tick) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an experimental PR, that refactors much of netfox's internals, inspired by Godot's server pattern.
While Godot had its own requirements, the interpretation here is that:
Hopefully this allows:
Also lays the foundations for some potentially wild features like:
TODO:
Pending fixes:
Sanity tasks: