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

Upgrade to bevy 0.10 #5

Closed
wants to merge 2 commits into from
Closed

Conversation

kulkalkul
Copy link
Contributor

@kulkalkul kulkalkul commented Mar 23, 2023

This is what I've done as a follow-up on #4

So, I've upgraded iced's wgpu in a fork:
https://github.com/kulkalkul/iced/commits/self-usage
I've done it, so it is non-breaking. Those expect calls there are similar in how it handles pre-wgpu 0.14. It is also handled the same in
https://github.com/sconybeare/wgpu_glyph/tree/wgpu-0.15

For bevy_iced, there are two changes I think you might be interested in:

  • I've changed Res<Windows> to Query<Window> and kept it a single window (I have no idea how I can do it for multiple windows).
  • Bevy decided to hide RenderContext fields, so they can only be accessed with functions ([Merged by Bors] - Support recording multiple CommandBuffers in RenderContext bevyengine/bevy#7248). But the command_encoder function is &mut self, so it isn't possible to get both device and command_encoder from RenderContext. So, I've got the RenderDevice from the world, which is OK but feels a little hacky tbh.

Aside from the above points, a bug causes flashing rendering for UI. It happens randomly, at a random rate, sometimes so slow it is unnoticeable. Here is the video of it:

FAST FLICKERING VIDEO, EPILEPSY/SEIZURE WARNING!

magellan_2023-03-23_11-28-49.mp4

I've figured that the new pipelined rendering implementation causes IcedNode::run to run parallel to IcedContext::display. As a result, the run function misses the IcedProp.did_draw change. I didn't know the project enough to solve it, so I decided to ask it.

I hope this isn't too long 😅

@kulkalkul
Copy link
Contributor Author

kulkalkul commented Mar 24, 2023

Each line is frame boundary. T(rue) and F(false) represents last state of did_draw. So T happens when IcedContext::display runs, F happens when IcedNode::run runs:
ApplicationFrameHost_2023-03-23_12-32-05

I thought about this for a while. I can only think of creating a did_draw state with three possible values. I've thought about finding another way to do this, as this approach delays all did_draw changes by one frame. But that's only possible to detect by knowing which order they will execute, which is impossible with new multithreaded rendering.

did_draw depends on user input set to true, but this is not the case for setting it to false. In theory, the user can state it, but in practice, having a function named "no_display" doesn't make sense. So, having a meaningful API to control this behaviour is impossible, so I accepted that a frame delay is the only fix.

@tasgon
Copy link
Owner

tasgon commented Mar 26, 2023

Thank you so much for the contribution. I was also a bit worried about the synchronization, so I tried a different trick. If you're interested, take a look at #6 and let me know which method you prefer.

Also, I'm fine with using your iced fork for now, but I will ask for a specific ref to pin those crates to. Is the current head on that branch okay?

@kulkalkul
Copy link
Contributor Author

I don't know how extracts are synchronized, but it works! 🥳 I prefer your version, as that's probably how this should've been.

I didn't do anything after that commit, so the current head is okay. I should probably open a PR to iced, but the change is so minimal I'm not sure if it is worth the hassle.

@tasgon
Copy link
Owner

tasgon commented Apr 16, 2023

Closing as #6 has been merged.

@tasgon tasgon closed this Apr 16, 2023
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.

2 participants