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

Limit draw updates #202

Merged
merged 15 commits into from
Sep 27, 2023
Merged

Limit draw updates #202

merged 15 commits into from
Sep 27, 2023

Conversation

graefjk
Copy link
Contributor

@graefjk graefjk commented Sep 23, 2023

This change limits the between individual calls of drawEntities(). This way drawEntities() is not called every time a new Entity is created. This can reduce Lag when multible entities are added to the PlayingField at the same time. The time between drawEntities() calls can be changed with the setTimeBetweenDraws method which accepts the time in milliseconds. The time is initially set as 32ms between draw calls, which equates to a maximum of 31.75 Draws/Second. This implements #194.

@graefjk graefjk marked this pull request as draft September 23, 2023 15:16
@graefjk graefjk marked this pull request as ready for review September 23, 2023 15:31
@graefjk graefjk marked this pull request as draft September 24, 2023 23:22
@graefjk graefjk marked this pull request as ready for review September 25, 2023 01:46
Copy link
Collaborator

@buehlefs buehlefs left a comment

Choose a reason for hiding this comment

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

Looks good on first glance. The alternative to implementing the update batching/debouncing in the simulation is to implement it in the playfield drawer

public void setDrawables(final List<Drawable> drawables) {

There you could use swing timers to trigger draw updates (this would be a bit nicer than the manual thread spawning and joining here). But I don't know where the biggest performance bottlenecks actually are.

@graefjk
Copy link
Contributor Author

graefjk commented Sep 26, 2023

I chose to implement this here instead of in the playfield drawer since drawEntities executes this.getAllEntities() every time it is called and this.getAllEntities() takes longer the more entities there are.

} catch (@SuppressWarnings("unused") final EntityNotOnFieldException e) {
//Entity has been removed from the field while this loop was running.
//Just don't draw it and ignore the exception.
if (this.awaitingEntityDraw) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually prefer the fast exit form over nesting everything in an if. This makes the method easier to read, as all the fail conditions are at the top and can be ignored for the rest of the method body.

if (!this.awaitingEntityDraw) {
    return; // no updates to draw
}
// rest of the method

Copy link
Collaborator

@buehlefs buehlefs left a comment

Choose a reason for hiding this comment

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

Looks good now. (But I did not run the code myself, so merge at your own risk.)

@graefjk
Copy link
Contributor Author

graefjk commented Sep 27, 2023

Looks good now. (But I did not run the code myself, so merge at your own risk.)

I cannot merge any pull request as i am not a maintainer of this project

@buehlefs
Copy link
Collaborator

It looks like I don't have the rights to make you a project maintainer. You should ensure that you have at least 1-2 active maintainers with the correct access rights (@neumantm should be able to help with that). If you want me to merge just leave me a thumbs up. (but I may not be available for emergency merges on a short notice)

@buehlefs buehlefs merged commit 37bb3e3 into master Sep 27, 2023
2 checks passed
@buehlefs buehlefs deleted the limitDrawUpdates branch September 27, 2023 13:06
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.

2 participants