refactor: Store state and inputs in HistoryBuffers as PropertySnapshots#399
refactor: Store state and inputs in HistoryBuffers as PropertySnapshots#399elementbound merged 30 commits intofoxssake:mainfrom
Conversation
Reinstated only core _HistoryBuffer functionality.
Refactored _get_history() into _HistoryBuffer.
Cleaned up comments, added typing where needed.
Added _PropertyStoreSnapshot, removing the need for PropertySnapshot from RollbackSynchronizer.
Fixed missing conversion.
Reformatted PropertyStoreSnapshot.
Added typing comments to RollbackSynchronizer dictionaries.
Added trim feature to HistoryBuffer and refactored out of RollbackSynchronizer.
Refactored _sanitize_by_authority into PropertyStoreSnapshot.
Updated comments.
Fixed if statement formatting.
| # Typed as Dictionary[int, _PropertyStoreSnapshot] | ||
| var _buffer: Dictionary = {} | ||
|
|
||
| func get_snapshot(tick: int, default = {}) -> _PropertyStoreSnapshot: |
There was a problem hiding this comment.
Is default still needed?
There was a problem hiding this comment.
Somehow missed that line 10 doesn't use default. I kept this to keep consistency with how Dictionary.get() works, with the intention of line 10 returning defualt, but we should be able to remove it if preferred.
| if _buffer.has(tick): | ||
| return _buffer[tick] | ||
| else: | ||
| return _PropertyStoreSnapshot.new() |
There was a problem hiding this comment.
I'm thinking whether returning just null would make sense, to avoid an extra alloc, but I'll have to read the rest of the PR.
There was a problem hiding this comment.
This is in line with how RollbackSynchronizer handled things previously, as exampled the following line:
_states[tick] = PropertySnapshot.merge(_states.get(tick, {}), full_state)
I'm not sure if anything else would break with it returning null.
| var closest_tick = get_closest_tick(tick) | ||
|
|
||
| if closest_tick == -1: | ||
| return _PropertyStoreSnapshot.new() |
There was a problem hiding this comment.
Similar comment here, might be worth returning a null here.
| var sender = multiplayer.get_remote_sender_id() | ||
| var sanitized = _sanitize_by_authority(state, sender) | ||
| var sanitize_success := deserialized.sanitize(sender, _property_cache) |
There was a problem hiding this comment.
Same note as with inputs above
Removed get_snapshot_as_dictionary, as it's succeeded by PropertyStoreSnapshot.as_dictionary().
Updated _HistoryBuffer as per @elementbound's PR comments.
Updated _PropertyStoreSnapshot as per @elementbound's PR comments.
Updated RollbackSynchronizer as per @elementbound's PR comments.
Fixed assigning empty state to HistoryBuffer if no valid diffs were transmitted.
Successor to #397, as it was easier to rewrite than bugfix. This PR has functional diff states, and a cleaner codebase implementing most requested changes from the previous PR.