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

Improve model loading messages (long-lived background thread) #1476

Merged

Conversation

zthompson47
Copy link
Contributor

This PR fixes #1431 and fixes #1327 by spawning a host background thread that lives outside of the the event loop. A handle is used inside the event loop to send new models for processing. Status messages are sent back to the event loop via winit::event_loop::EventLoopProxy.

hannobraun
hannobraun previously approved these changes Jan 4, 2023
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @zthompson47, this is great!

I tested this, and it works great. I also reviewed the code and it looks good. I left comments where things jumped out to me, but none of that is a blocker.

You said that you wanted to do more cleanup and write tests before merging. While that would be very welcome, I'd be more than happy to merge this as-is. It is a clear improvement, after all, fixing two issues!

Additional cleanup and testing can be submitted in follow-up pull requests. This would also reduce the likelihood of other pull requests causing merge conflicts here in the meantime. What do you think?

One caveat: I haven't looked at #1477 yet (will do so soon), so maybe I'll end up liking that better. However, based on our conversation in #1431, I believe that the "long-lived thread" approach from this pull request is likely to be the better one.

crates/fj-host/src/evaluator.rs Outdated Show resolved Hide resolved
crates/fj-host/src/host.rs Outdated Show resolved Hide resolved
crates/fj-host/src/host.rs Outdated Show resolved Hide resolved
crates/fj-host/src/host_handle.rs Outdated Show resolved Hide resolved
crates/fj-host/src/host_handle.rs Outdated Show resolved Hide resolved
crates/fj-host/src/watcher.rs Outdated Show resolved Hide resolved
crates/fj-window/src/event_loop_handler.rs Show resolved Hide resolved
crates/fj-window/src/event_loop_handler.rs Outdated Show resolved Hide resolved
@@ -3,17 +3,19 @@
//! Provides the functionality to create a window and perform basic viewing
//! with programmed models.

#![allow(clippy::result_large_err)]
Copy link
Owner

Choose a reason for hiding this comment

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

See previous comment about #![allow(...)].

crates/fj-host/src/host.rs Outdated Show resolved Hide resolved
@zthompson47
Copy link
Contributor Author

Thanks for review and comments - very helpful!

| Additional cleanup and testing can be submitted in follow-up pull requests
That sounds good to me. I think your comments addressed most of the cleanup I was thinking of, and the tests might end up being a bit of a project.

I'll resolve the above conversations and we can merge this in.

@hannobraun hannobraun marked this pull request as ready for review January 6, 2023 11:27
`Host` now runs in a background thread and moves model processing out of
the `winit` event loop. It controls more of the model loading process
and can provide better status messages to display to the user.
This commit also fixed the timing of the `StartWatching` status message.
Instead of exposing the separation of a host background thead and an
associated handle, create a single host constructor to use in the public
api.
The struct names were changed, so rename the files accordingly.
@hannobraun hannobraun force-pushed the improve-model-loading-messages-wip branch from 0773c4c to 815dea7 Compare January 6, 2023 11:28
Copy link
Owner

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Thank you, @zthompson47, those latest changes look great!

I just did a rebase to get this up-to-date with main, and will merge as soon as CI is green.

@hannobraun hannobraun enabled auto-merge January 6, 2023 11:29
@hannobraun hannobraun merged commit bd40f1d into hannobraun:main Jan 6, 2023
@zthompson47
Copy link
Contributor Author

Thanks for all the help getting that PR merged!

I never did that last edit to sort out the issue with #![allow(clippy::result_large_err)]. Still getting used to the workflow on github, but sounds like I'll just wrap that up and put it in a new PR (with no issue filed).

@zthompson47 zthompson47 deleted the improve-model-loading-messages-wip branch January 6, 2023 14:19
@hannobraun
Copy link
Owner

Thanks for all the help getting that PR merged!

No problem. It was you who did all of the work!

I never did that last edit to sort out the issue with #![allow(clippy::result_large_err)]. Still getting used to the workflow on github, but sounds like I'll just wrap that up and put it in a new PR (with no issue filed).

Different projects handle that kind of stuff differently. Myself, I'm pretty flexible in how to do things, as I want to encourage contributions, and I don't think it's reasonable to expect every contributor to read a thick "how we do it" handbook. That said, these are my preferences:

  • Open pull requests basically are a burden for everyone involved. Mostly for their author, because as long as they stay open, they tend to accumulate merge conflicts that need to be fixed, as more changes are being made to the main branch. That's why I prefer to merge them as soon as I feel they are an improvement, even if they have problems.
  • If I merge a pull request with problems that I feel are important, I open an issue right after merging. That way, we can be sure the problem isn't forgotten, even if it's not handled right away.
  • Like open pull requests, open issues also have a cost. The more you have, the harder it becomes to stay on top of them, and the more likely they are to get out of date. At the extreme end, it becomes easier to just test the app and look at the code to figure out what can be improved, than sorting through issues.
  • I think the #![allow(clippy::result_large_err)] thing isn't quite important enough to warrant an open issue. It would still be a welcome improvement, but it's not the kind of thing we need to track through an open issue. If you wanna do it, great! Just open another pull request. If not, someone else can improve it whenever they come across it.

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.

Improve initial model loading message Shape processing is blocking event loop
2 participants