-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Display background & moving progress bar on shutdown screen #14597
Conversation
311e745
to
98200bb
Compare
Assigning @grorp due to being the author of the issue (unsubscribing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works. Simple and effective. I like this. This may not be an ideal "striped" progress bar (and hence slightly confusing when it wraps around), but it's definitely good enough and much better than the status quo.
To avoid the confusion without making things overly complicated, I'd suggest this: progress.bar.mp4(My implementation of this is at https://github.com/grorp/minetest/commits/shutdownScreen, but the patch is horrible and not meant to be used as is.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The far edges of the clouds on the shutdown screen are black or sometimes green-ish for me. This doesn't happen on the loading screen.
I've only taken a very brief look at your commits as of right now, but I think at the point where you are modifying the rendering engine itself, it makes more sense to pass a boolean, which is a false by default in order to control wether the loading bar will do it's indefinate loading animation instead of providing the option of drawing a partional bar only. Anyway, I'll try to actually read through the entire thing and implement your desired changes tomorrow :) |
inspired by group's implementation
I have not been able to reproduce that, is it still the case on my most recent commit? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle, your approach is also ok, but it looks bad at the start and the end of the progress bar because of the way the texture is mapped to the slider. Example:
I think it would look best if both ends of the slider would always use the ends of the texture.
It's also irritating that the progress bar doesn't start at the beginning but at its last position if you start and shut down multiple games. It should start at the beginning again.
I have not been able to reproduce that, is it still the case on my most recent commit?
Yes, it is.
I have now (finally) been able to reproduce this behavior! I noticed that the color of the distant clouds is always the color the sky was before the shutdown, for example, if you run Any chance it was night in your game session? |
not fixed EDIT: also misclicked |
This needs an update too: Line 1426 in 553ea89
|
The only solution I can think of is passing a pointer to a variable the position value is stored in then, are you fine with that? |
At this point, I've made way more commits than I feel like is reasonable for a PR of this scale, so, now may be a good time for a squash. |
1cb0286
to
23f77c8
Compare
23f77c8
to
6bc06fd
Compare
To be honest, I think in the long term it might be better altering Edit: here is a patch (I had to change the file extension so GitHub allows me to upload it) for the said change, since it ended up being a minor change, maybe it's worth applying it to the PR? |
src/client/game.cpp
Outdated
|
||
fps_control.reset(); | ||
f32 timer = 0.0f; | ||
|
||
while (!client->isShutdown()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, this loop will never run anyway. client->Stop()
above sets m_shutdown
to true ...
minetest/src/client/client.cpp
Lines 314 to 316 in 07fe8d4
void Client::Stop() | |
{ | |
m_shutdown = true; |
... and client->isShutdown()
returns true as soon as m_shutdown
is true:
minetest/src/client/client.cpp
Lines 331 to 334 in 07fe8d4
bool Client::isShutdown() | |
{ | |
return m_shutdown || !m_mesh_update_manager->isRunning(); | |
} |
Anyway, that's out of scope for this PR to investigate and fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accordingly, I've removed the changes to this loop again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, assuming that boolean is not volatile, shouldn't the entire loop be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not remove it without investigating first:
- what's the purpose of the code?
- did it ever work?
- why doesn't it work (anymore)?
- is there a bug because the code doesn't work?
which is likely out of scope for this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've applied some cleanup to this PR, also including your patch for showOverlayMessage
. It looks good to me now.
At this point, I've made way more commits than I feel like is reasonable for a PR of this scale, so, now may be a good time for a squash.
You don't have to squash, the PR will be squashed automatically when merging.
This Pull Request adds the main menu background to the shutdown screen, and makes the progress bar constantly refill itself indefinitely as long as the game is still shutting down. It does so by using a load screen instead of an overlayMessage.
Fixes #14571
Ready for Review.
shutdown.webm
How to test