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

Cursor Lerp Smoothing #223

Merged
merged 3 commits into from
Aug 21, 2023
Merged

Cursor Lerp Smoothing #223

merged 3 commits into from
Aug 21, 2023

Conversation

ewoudje
Copy link
Contributor

@ewoudje ewoudje commented Aug 21, 2023

Adds smoothing for cursor.
Its a simple lerp between the prev and newest packet.
This means that the cursor is always acting 1 packet behind but this should be fine.

@RatDotRaw
Copy link

Looks good.

@polycone
Copy link
Member

The main issue is that all data related to the position evaluation and the evaluation itself should not be in the player state (broken SRP - high maintainability cost).
The overall approach is good, a packet behind thing is acceptable I guess.
So, please consider to move the evaluation and its data into DrawCursorComponent.

Copy link
Member

@polycone polycone left a comment

Choose a reason for hiding this comment

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

Please address this issue first: #223 (comment)

@ewoudje
Copy link
Contributor Author

ewoudje commented Aug 21, 2023

The main issue is that all data related to the position evaluation and the evaluation itself should not be in the player state (broken SRP - high maintainability cost). The overall approach is good, a packet behind thing is acceptable I guess. So, please consider to move the evaluation and its data into DrawCursorComponent.

Yea I can move the getter code to the drawing, the only problem is that the data has to be modified when receiving the packet.
I'm unsure how I could solve this cleanly.

@polycone
Copy link
Member

That's a good question, I was thinking for a while, seems like the cursor time must be a part of the state for sure.
I think it's time to replace specific CursorPosition and create a new record PlayerCursor with Position and Time properties and replace PlayerState.CursorPosition with just public PlayerCursor Cursor { get; set; } = new(...); for example.
Then the time should be included in the UpdateCursorPosition command. I'm not sure if Unity's Time.time can be used tho, because it could be scaled. Maybe DateTime.Now.Ticks is more reliable.
Then you can detect changes based on time, store previous values and evaluate the position locally in the DrawCursorComponent.

Btw, thanks for the contribution! That's really nice to see🙂

@ewoudje
Copy link
Contributor Author

ewoudje commented Aug 21, 2023

No problem for the contribution.
I was very happy to see that a multiplayer mod existed.
I've always wanted to play ONI co-op. So ofc I gotta help :P

Copy link
Member

@polycone polycone left a comment

Choose a reason for hiding this comment

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

Nice work👍

@polycone polycone merged commit febab3e into onimp:main Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants