Rollback Refactor - _HistoryBuffer Addition by DustieDog · Pull Request #397 · foxssake/netfox

DustieDog · 2025-02-05T14:13:55Z

Draft PR for #385.

Currently the client is bugged, resulting in local artifacts and unintended behaviour on the host (Client player doesn't move smoothly, and moves faster than expected in Forest Brawl)

Bugged but complete RollbackSync

Data now properly sends over RPC. Introduces a bug where clients move faster than expected, and also get local artifacting.

DustieDog · 2025-02-05T14:18:54Z

addons/netfox/properties/property-store-snapshot.gd

Plan to rename this PropertySnapshot prior to the rollback refactor being merged to main. Using unique name for now to avoid potential conflicts with existing property-snapshot.gd references until we're sure it's safe to switch.

elementbound

Kudos for the PR! Added some comments. Feel free to leave the unit tests for the end, I'm happy to add them.

elementbound · 2025-02-06T20:01:09Z

addons/netfox/properties/history-buffer.gd.gd

I see this class is already specialized to snapshots. Is the original HistoryBuffer > PropertyHistoryBuffer combo intentionally skipped?

Also note to self, since this is now refactored into its own class, we can now add unit tests!

I see this class is already specialized to snapshots. Is the original HistoryBuffer > PropertyHistoryBuffer combo intentionally skipped?

PropertyHistoryBuffer is just a dictionary. Taking your previous comment:

No, that's PropertyHistoryBuffer. The distinction is analogous to List<T> vs. List<StateSnapshot>

HistoryBuffer is just a Dictionary[String, PropertyStoreSnapshot], there's nothing I can see that can be refactored out of that which a Dictionary[String, T] doesn't already do. If a use case pops up later for something of the sort we can look at it then though.

I'd prefer to have a generic collection class ( HistoryBuffer ), and a specialized one extending it, that we use. Part of it is pure preference of having generic collections, part of it is that it would enable netfox to provide users a way to associate arbitrary data ( not just snapshots ) with ticks.

there's nothing I can see that can be refactored out of that which a Dictionary[String, T] doesn't already do
I believe merge is one such method that is specific to property snapshots.

addons/netfox/properties/property-cache.gd

addons/netfox/properties/property-store-snapshot.gd

elementbound · 2025-02-06T20:04:48Z

addons/netfox/properties/property-store-snapshot.gd

+extends Object
+class_name PropertyStoreSnapshot
+
+# Typed as Dictionary[String, Property]


While at it, what do you think about semantically documenting dictionaries? E.g.:

Suggested change

# Typed as Dictionary[String, Property]

# Maps property path strings to Property instances

Dictionary[String, Property] is more direct and therefore readable to me personally, perhaps both are warranted?

addons/netfox/properties/property-store-snapshot.gd

elementbound · 2025-02-06T20:49:18Z

addons/netfox/rollback/rollback-synchronizer.gd

-		var input := inputs[offset] as Dictionary
-		if input == null:
+
+		var input := PropertyStoreSnapshot.deserialize(inputs[offset] as Dictionary)


Is the cast needed here?

It was at one point to fix an RPC error, further testing is needed to see if it's still required.

elementbound · 2025-02-06T20:52:39Z

addons/netfox/rollback/rollback-synchronizer.gd

+
+		var input := PropertyStoreSnapshot.deserialize(inputs[offset] as Dictionary)
+
+		if input.size() == 0:


This got swapped with the original null-check. The original idea was to prevent against accidental nulls present in the received input array.

I'd recommend something like this instead:

Suggested change

var input := PropertyStoreSnapshot.deserialize(inputs[offset] as Dictionary)

if input.size() == 0:

var raw_input := inputs[offset] as Dictionary

if raw_input == null:

# We've somehow received a null input - shouldn't happen

_logger.error("Null input received for %d, full batch is %s", [input_tick, inputs])

continue

var input := PropertyStoreSnapshot.deserialize(raw_input)

Note: Couldn't comment on the unchanged parts so this is not safe to commit as-is 😄

What are your thoughts of moving this into PropertyStoreSnapshot.deserialize() instead?

elementbound · 2025-02-06T20:56:51Z

addons/netfox/rollback/rollback-synchronizer.gd

 		if not sanitized.is_empty():
-			var known_input := _inputs.get(input_tick)
+			var known_input := _inputs.get_snapshot(input_tick)
+			# TODO: Does this work with PropertyStoreSnapshot?


Probably not, as these are compared by reference IIRC. A unit test would decide though 😄 I'm not aware of any way to override the == operator in Godot, so our best bet to write a .equals() method.

elementbound · 2025-02-06T20:57:24Z

addons/netfox/rollback/rollback-synchronizer.gd

 		return
-
+
+	var deserialized = PropertyStoreSnapshot.deserialize(state)


Suggested change

var deserialized = PropertyStoreSnapshot.deserialize(state)

var deserialized := PropertyStoreSnapshot.deserialize(state)

addons/netfox/rollback/rollback-synchronizer.gd

DustieDog

Reviewed @elementbound's notes, no disagreements.

DustieDog · 2025-02-05T14:21:08Z

addons/netfox/rollback/rollback-synchronizer.gd

+	_states.clear_history()
+	_inputs.clear_history()


Should calls to non-dictionaries follow dictionary conventions? IE should these functions be named clear()?

Good catch, missed this during the in-depth review. I'd prefer that, especially since clearing history is a given for a history buffer.

DustieDog · 2025-02-06T21:11:35Z

addons/netfox/properties/history-buffer.gd.gd

I see this class is already specialized to snapshots. Is the original HistoryBuffer > PropertyHistoryBuffer combo intentionally skipped?

PropertyHistoryBuffer is just a dictionary. Taking your previous comment:

No, that's PropertyHistoryBuffer. The distinction is analogous to List<T> vs. List<StateSnapshot>

HistoryBuffer is just a Dictionary[String, PropertyStoreSnapshot], there's nothing I can see that can be refactored out of that which a Dictionary[String, T] doesn't already do. If a use case pops up later for something of the sort we can look at it then though.

addons/netfox/properties/property.gd

addons/netfox/rollback/rollback-synchronizer.gd

DustieDog · 2025-02-06T21:40:03Z

addons/netfox/rollback/rollback-synchronizer.gd

-		_retrieved_tick = earliest_tick
-		return buffer[earliest_tick]
+func _sanitize_by_authority(snapshot: PropertyStoreSnapshot, sender: int) -> PropertyStoreSnapshot:
+	var sanitized := PropertyStoreSnapshot.new()


I thought I read somewhere that modifying dictionaries in a for loop can cause issues, but I can't find a reference.

DustieDog · 2025-02-06T21:40:55Z

addons/netfox/rollback/rollback-synchronizer.gd

-		var input := inputs[offset] as Dictionary
-		if input == null:
+
+		var input := PropertyStoreSnapshot.deserialize(inputs[offset] as Dictionary)


It was at one point to fix an RPC error, further testing is needed to see if it's still required.

DustieDog · 2025-02-06T21:45:35Z

addons/netfox/rollback/rollback-synchronizer.gd

+
+		var input := PropertyStoreSnapshot.deserialize(inputs[offset] as Dictionary)
+
+		if input.size() == 0:


What are your thoughts of moving this into PropertyStoreSnapshot.deserialize() instead?

addons/netfox/rollback/rollback-synchronizer.gd

elementbound · 2025-02-06T22:29:48Z

addons/netfox/properties/history-buffer.gd.gd

+func get_earliest_tick() -> int:
+	return _history.keys().min()
+
+func get_latest_tick() -> int:
+	return _history.keys().max()


It might make sense to cache these, if these methods keep being used in the final PR. Needs benchmarking.

addons/netfox/rollback/rollback-synchronizer.gd

addons/netfox/properties/property-store-snapshot.gd

addons/netfox/rollback/rollback-synchronizer.gd

DustieDog · 2025-02-12T20:04:28Z

Closing in favour of #399.

DustieDog added 2 commits February 3, 2025 23:04

Bugged RollbackSync

56e3090

Bugged but complete RollbackSync

Bugfix: Data Over RPC

fb7fef6

Data now properly sends over RPC. Introduces a bug where clients move faster than expected, and also get local artifacting.

DustieDog commented Feb 5, 2025

View reviewed changes

DustieDog marked this pull request as draft February 5, 2025 14:33

elementbound changed the base branch from epic/refactor-rollback-synchronizer to main February 5, 2025 20:19

elementbound changed the base branch from main to epic/refactor-rollback-synchronizer February 5, 2025 20:20

elementbound reviewed Feb 6, 2025

View reviewed changes

DustieDog commented Feb 6, 2025

View reviewed changes

elementbound reviewed Feb 6, 2025

View reviewed changes

elementbound added 2 commits February 7, 2025 22:32

update rollback debugger

0765003

fix memory leak and input age

3523d61

elementbound reviewed Feb 7, 2025

View reviewed changes

addons/netfox/rollback/rollback-synchronizer.gd Show resolved Hide resolved

elementbound reviewed Feb 7, 2025

View reviewed changes

addons/netfox/properties/property-store-snapshot.gd Outdated Show resolved Hide resolved

fix diff state issue and add tests

8f23dca

DustieDog commented Feb 8, 2025

View reviewed changes

addons/netfox/rollback/rollback-synchronizer.gd Show resolved Hide resolved

DustieDog mentioned this pull request Feb 12, 2025

refactor: Store state and inputs in HistoryBuffers as PropertySnapshots #399

Merged

DustieDog closed this Feb 12, 2025

DustieDog deleted the Rollback-Refactor branch February 12, 2025 20:52

	# Typed as Dictionary[String, Property]
	# Maps property path strings to Property instances


		var input := PropertyStoreSnapshot.deserialize(inputs[offset] as Dictionary)

		if input.size() == 0:

-		var input := PropertyStoreSnapshot.deserialize(inputs[offset] as Dictionary)
-		if input.size() == 0:
+		var raw_input := inputs[offset] as Dictionary
+		if raw_input == null:
+			# We've somehow received a null input - shouldn't happen
+			_logger.error("Null input received for %d, full batch is %s", [input_tick, inputs])
+			continue
+		var input := PropertyStoreSnapshot.deserialize(raw_input)

		return


		var deserialized = PropertyStoreSnapshot.deserialize(state)

	var deserialized = PropertyStoreSnapshot.deserialize(state)
	var deserialized := PropertyStoreSnapshot.deserialize(state)

Uh oh!

Conversation

DustieDog commented Feb 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elementbound left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DustieDog left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DustieDog commented Feb 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants