Skip to content

timer objects now require delta-time - #191

Open
tank-king wants to merge 1 commit into
sloukit:mainfrom
tank-king:timer-dt
Open

timer objects now require delta-time#191
tank-king wants to merge 1 commit into
sloukit:mainfrom
tank-king:timer-dt

Conversation

@tank-king

Copy link
Copy Markdown
Collaborator

NOTE: This PR is incomplete, meant for feedback and suggestions from reviewers before a heavy refactoring of the codebase

Summary

This PR implements delta-time into the Timer class, so they adjust according to the speed factor of the game. This is useful for slow-motion or fast-forward capabilities as described in the PR #189

Checklist

  • I have made sure that the code follows the formatting and style guidelines of the project.

Labels

type: enhancement

@tank-king

Copy link
Copy Markdown
Collaborator Author

NOTE: If this update is approved, then a heavy refactoring of all usages of Timer objects in the codebase is required.

@sloukit

sloukit commented Oct 9, 2024

Copy link
Copy Markdown
Owner

@DangerousVanilla could you review this PR, please?

@DangerousVanilla

Copy link
Copy Markdown
Collaborator

Yeah, on it now - my apologies for the delay.

@DangerousVanilla DangerousVanilla left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@tank-king / @sloukit: So I reviewed the code here and don't have any issues with it specifically. I also generally prefer a delta time implementation for a variety of reasons, but given the state of the project and the coming deadlines for release I feel a significant refactor at this point may not be worth the time for the purpose of this project, or at least the time invested in that may be better utilized elsewhere.

Unless there is a significant desire for an ability to change game speed in multiple other areas I'd recommend just leaving it as is, especially after the successful cutscene fast-forward feature request implementation in PR189 by @tank-king. Maybe if we come to a point where everything is considered working and good enough for the intended study to be performed we could pick this up as an enhancement?

@sloukit, if you feel differently please let me know!

@tank-king

Copy link
Copy Markdown
Collaborator Author

Sounds good, my main intention was to keep this PR ready in case things glitch out in the game, we can ignore this and keep on low priority for now until something actually breaks 👍

@sloukit

sloukit commented Oct 14, 2024

Copy link
Copy Markdown
Owner

@DangerousVanilla Thank you very much for your review. I totally agree with you - we should finish the most important features and keep this PR when we start optimising. Also @tank-king, thank you!

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