Conversation
Position and get layer based on a egui::Response object
Deprecated and not needed in updated egui
Includes a PR we got merged to fix flickering
Needed to avoid multiple egui versions
Missed it before. It was pulling in bevy 0.16 still
For big_space and bevy_editor_cam
There was a problem hiding this comment.
@shanecelis @zxq82lm I did a bit of a pre-review leaving some comments to explain a few things. Here's the bevy migration guide for reference:
Note that this is ready for review even though clippy is broken. See comment about WINIT_WINDOWS below.
Another thing to be aware of is this brought some changes to macOS that I'm unable to test locally.
When testing, I recommend especially banging on the viewport, and trying out all our widgets. These changes affect pretty much everything.
| match self { | ||
| LabelSource::Label(l) => l.clone(), | ||
| LabelSource::System(system) => system.run(filter, world), | ||
| LabelSource::System(system) => system.run(filter, world).expect("Missing label"), |
There was a problem hiding this comment.
This matches the old behavior, which would panic. We may want to handle explicitly.
| cam.hdr = hdr_enabled.0; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
HDR is now a component instead of Camera attribute.
There was a problem hiding this comment.
This is fine. It'll work but I feel like we could add some qualifiers like Added<Camera>, and checking whether HdrEnabled.is_changed() could be used to opt out early. But our camera number is a handful and it doesn't do anything in the loop that isn't necessary, so again it's fine.
| self.prompt = Some(prompt.to_string()); | ||
| self | ||
| } | ||
|
|
There was a problem hiding this comment.
All of the into_event calls are using the method below.
| use objc2::{ClassType, msg_send, msg_send_id}; | ||
| use objc2_app_kit::{NSColor, NSToolbar, NSWindow, NSWindowStyleMask, NSWindowToolbarStyle}; | ||
|
|
||
| WINIT_WINDOWS.with_borrow(|winit_windows| { |
There was a problem hiding this comment.
WINIT_WINDOWS replaces the old WinitWindows resource, using a scoped function now. This indents a lot of code that otherwise hasn't changed. What I'm doing for now is not indenting properly. clippy will complain, but it makes a lot less code to review. Once review is done I'll push the indentation changes.
| } | ||
| } | ||
|
|
||
| fn init_line_pipeline( |
There was a problem hiding this comment.
| let logo_full = state | ||
| .contexts | ||
| .add_image(state.images.logo_full.clone_weak()); | ||
| .add_image(EguiTextureHandle::Weak(state.images.logo_full.id())); |
There was a problem hiding this comment.
|
This looks pretty great, Anders! I did find a small issue on macOS. I fixed it and pushed a commit directly in this branch. I exercised the ball, drone, cube_sat, and three_body, and rocket examples with no problems. EDIT: Rocket works too. |
| cam.hdr = hdr_enabled.0; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This is fine. It'll work but I feel like we could add some qualifiers like Added<Camera>, and checking whether HdrEnabled.is_changed() could be used to opt out early. But our camera number is a handful and it doesn't do anything in the loop that isn't necessary, so again it's fine.
|
|
||
| fn setup_window_icon( | ||
| _windows: Query<(Entity, &bevy::window::PrimaryWindow)>, | ||
| // this is load bearing, because it ensures that there is at |
There was a problem hiding this comment.
This is a comment I'm sad to see go just because it looks like it's trying to prevent an accidental removal. Is it still true? It looks like it's not true any longer. Is that correct?
There was a problem hiding this comment.
Yeah I was torn on this too. Originally I had actually added an additional comment. When I looked closer I assumed that !_windows.is_empty() would be enough to ensure a window, but maybe not. It's also unclear why both parameters start with an underscore. We could defensively trust in Chesterton's Fence and keep using WINIT_WINDOWS here, or we could yolo and see if we ever have issues with random crashes on startup. I don't think that would be particularly hard to debug if it happened. I'm good either way, so your call.
There was a problem hiding this comment.
I just realized my thinking _windows might be sufficient makes no sense, because it's not even used for macOS. I vote we defensively use WINIT_WINDOWS with an even more verbose comment.
There was a problem hiding this comment.
@shanecelis I just pushed the closest port I can come up with for the old behavior, along with a comment. If it looks good to you and you wouldn't mind verifying that it doesn't break macOS, I think we can resolve this.
|
@shanecelis only had one followup question. Once that resolved I'll push a new commit that does nothing other than clippy/fmt. |
|
It's working on macOS, but I am seeing this strange thing in the rocket example: 
|
|
Uh oh. uh-oh.mov |
|
Well that's weird. I'm seeing the panes not close properly on main as well, but the shared ID thing is unique to my branch. How did you get that nice warning widget about the IDs? |
After updating to bevy 0.17 we noticed that two of the plots on the rocket example had lock icons that were tied together, due to sharing the same egui ID. This is because we were generating the IDs from the raw pointer of the graph_state, and apparenting something about the bevy 0.17 changes is causing the address to be reused. This commit switches to using the bevy Entity as the seed for the egui ID, which should remain stable for as long as the plot exists.
|
I didn't do anything special to get the warning. I'm just lucky I guess. :) |
No description provided.