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

Split Sdl2Application mainLoopIteration #577 #580

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

AndreasLrx
Copy link

Split Sdl2Application mainLoopIteration in:

  • mainLoopEventIteration: process the application's events
  • mainLoopTickEventIteration: calls tickEvent() if implemented
  • mainLoopDrawEventIteration: calls drawEvent() if the redraw flag is set, and unset it

I did not split up the mainLoopIteration of the GlfwApplication/AbstractXApplication yet because the tickEvent isn't implemented in either of them.
Should I omit the mainLoopTickEvent() or implement the tickEvent() for the others applications with the same implementation as the one in Sdl2Application ?
Or either don't even split the mainLoopIteration ? (I didn't looked at their usages and therefore don't know if it might be usefull or not)

Linked: #577

@codecov
Copy link

codecov bot commented Aug 6, 2022

Codecov Report

Merging #580 (ac2351b) into master (e5d3ec8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #580   +/-   ##
=======================================
  Coverage   82.53%   82.53%           
=======================================
  Files         531      531           
  Lines       37613    37613           
=======================================
  Hits        31043    31043           
  Misses       6570     6570           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mosra mosra added this to the 2022.0a milestone Aug 29, 2022
@mosra mosra added this to TODO in Platforms via automation Aug 29, 2022
@mosra mosra linked an issue Aug 29, 2022 that may be closed by this pull request
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

Terribly sorry for taking ages to reply -- I had just a week off but the pile of emergency things to look at grew a lot during that time, so I only got to this PR now.

Oh and thank you for willing to implement this change for the other applications as well, I appreciate that.

Comment on lines 985 to 991
bool Sdl2Application::mainLoopTickEventIteration() {
if(!(_flags & Flag::NoTickEvent)) {
tickEvent();
return true;
}
return false;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I thought about this for a bit, and personally I wouldn't even add any mainLoopTickEventIteration() -- keep the code directly inside mainLoopIteration().

The point of tickEvent() is to run something on every event loop iteration even if the screen doesn't need to be updated (such as in regular apps that refresh their window only on input events) because with the implicit event loop there's no other way to achieve that. But if you have your own event loop handling anyway, you can just call that code directly instead of implementing some virtual interface.

That should also answer your question about what to do for Application implementations that don't have a tickEvent(). (Though, implementing tickEvent() for GLFW at least would be nice too, but that'd need adding also setMinimalLoopPeriod() and handling all possible interactions with draw and input events, and I don't want to force you to do all that work as well ;))

@mosra
Copy link
Owner

mosra commented Sep 13, 2022

This is awesome, thank you. I have no other comments, I'll get to merging this hopefully during the weekend.

@AndreasLrx AndreasLrx marked this pull request as ready for review September 15, 2022 06:59
@AndreasLrx
Copy link
Author

Sorry I forgot to remove the PR from draft. I might suggest other changes when working on my project ;).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Platforms
  
TODO
Development

Successfully merging this pull request may close these issues.

[Feature Request] Split mainLoopIteration() methods
2 participants