Implement Main Menu With Egui #38
Conversation
Yes, please do that and link back here from that issue so we’ll see the backlink. |
|
There we go, created emilk/egui#1790. |
|
The font rendering issue is something I could spend a little time investigating how to fix in Egui. I found a lead on what the issue might be, but if this isn't relatively important right now, I could just continue more work on Punchy instead. @erlend-sh or @odecay do you think this is something I should look into now, or later? |
|
Yeh just keep it going as a casual side-track, no need to chase it down to its end any time soon. |
|
Alright I think this is looking good, I appreciate all the comments you left, made the code easier to review. I'm not really familiar enough with egui to have feedback on that section but I am happy to see the image backed frames + buttons are working. In terms of the empty handles during loading I think that makes sense. Also I noticed load_fighters is being run in GameState::InGame, does it fit best there or is it something leftover from before? |
Load fighters only has anything to do if there are fighters in the world, which should only be while the game is playing, at least so far. So I think it makes sense right now, but it could change later, when we have player selection screens and stuff like that. |
| /// | ||
| /// This is run in a chain as either `not_hot_reload().chain(load_game)` or | ||
| /// `is_hot_reload().chain(load_game)` so that logic for hot reloading the game and loading the game | ||
| /// can be shared. | ||
| fn load_game( |
There was a problem hiding this comment.
Just to make sure I'm understanding correctly, this runs in loading state and then if hot reload is enabled/configured also in a hot reload state with some conditional logic that determines what to do in the hot reload case?
There was a problem hiding this comment.
Yep, exactly.
Since all the logic was almost exactly the same for both an initial load and hot reload it seemed reasonable to run the same system for both cases and just check an extra is_hot_reload flag.
Granted, it did make it a little more complicated, and I wasn't 100% sure it was worth it, but I think it's better than writing most of the same code twice and adding more opportunities for bugs where the hot reload runs different than the initial load.
There was a problem hiding this comment.
It was kindof unintuitive to follow at first but I think it makes sense. I wonder if it could be split up into a needs_hot_reload() iyes_loopless run condition which is then applied to a conditionset that runs the hotreload logic in its own system before the load system?
There was a problem hiding this comment.
could be left for a future pr though if it even makes sense
There was a problem hiding this comment.
Yeah, I think we could try it layer if we wanted to. I was just analyzing to try and see if it was possible/beneficial.
The issue I see with it is the code that needs to run only when it isn't a hot reload. It'd need to be split up into somewhat more independent systems, which might be a better design anyway, but yeah, I'll probably try that in a separate PR after this one.
|
I just finished a slight update which adds the pause menu, do you want me just to add that commit to this or open another PR? It's mostly just the implementation of the pause menu UI system, but also a small change to where the font sizes are stored in the assets. |
I say open another if it doesn't really implicate what is in this one. |
|
Got it. 👍 |
|
I dont think there is much point delaying getting this in, lemme do that then open the pause menu PR. |
This implements the main menu with Egui.
An organizational change I made you might want to be aware of before reviewing is I removed the distinction between the
Gameasset and theGameMetastructs. Now everything is just inGameMeta. It started to get really cumbersome to have the loaded assets inside ofGamein a separate tree from theGameMetawhere all the non-asset stuff was held. What I did was move all of the asset handles inside of theMetastructs, and didserde(skip)on them all, so they will be loaded with initially meaningless values, such as nullHandle<Image>'s.After the asset is deserialized and it's corresponding
load_<asset_name>()system has run, the handles will be replaced with meaningful values.Technically I could have used
Option<Handle<Image>>instead, but that would require unwraping all over the place in systems where we know they should only run once the data has been loaded. By just using deadHandles, we get predictable behavior without having tounwrap().On the UI front, it's worth noting that Egui doesn't render perfectly crisp fonts when you use pixel-style fonts most of the time. You have to kind of get the screen size and position just right if you want to get perfect lines. Otherwise it ends up a little blurred at the edges, just across 2 pixels. In Bevy Retrograde I fixed that by creating a custom bitmap font loader and renderer, but that's a bit more work to integrate/use and I'm not sure if you guys are really set on the pixel fonts or not anyway. It just something for future discussion, and maybe opening an egui issue/disucssion to explore whether or not egui can just fix it anyway.
Anyway, this is a big one, so let me know what you think! I'll start working on the pause menu on top of this, which should go relatively quickly, tomorrow I think.