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

Fix multiple partial updates of the same texture #1338

Merged
merged 2 commits into from Jun 19, 2022
Merged

Conversation

wucke13
Copy link
Contributor

@wucke13 wucke13 commented Mar 7, 2022

introduces a queue for texture changes

Closes #1337.

@@ -150,7 +153,7 @@ impl TextureMeta {
#[must_use = "The painter must take care of this"]
pub struct TexturesDelta {
/// New or changed textures. Apply before painting.
pub set: AHashMap<TextureId, ImageDelta>,
pub set: AHashMap<TextureId, Vec<ImageDelta>>,
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe it would be even simpler to just make this a Vec<(TextureId, ImageDelta)>? I don't really see the point of having it be a hashmap anymore.

Copy link
Contributor Author

@wucke13 wucke13 Mar 8, 2022

Choose a reason for hiding this comment

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

Excellent suggestion. That avoids the double allocations. However, we would then always push to the end, and pop the front, like a FIFO, right? If so, maybe using a VecDequeue would be clever? On the other hand, we always consume the full Vec at once when we consume it, so using into_iter() will probably be even more efficient than the VecDequeue, then again. Does into_iter() on a normal vector cause any de-allocations?

Copy link
Owner

Choose a reason for hiding this comment

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

A Vec is the best fit since we always push to the back and consume the whole thing with either std::mem::take or drain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really see the point of having it be a hashmap anymore.

On argument for this: we can remove all deltas accumulated when a full update is received for a specific id. However this is quite insane API usage, requesting multiple changes of the same image only to toss it completely later on by a full update.

@emilk emilk changed the title fix #1337 Fix multiple partial updates of the same texture Mar 8, 2022
@emilk
Copy link
Owner

emilk commented Apr 3, 2022

I think we should make it a Vec<(TextureId, ImageDelta)> for simplicity sake. Are you still planning on working on this, or should I take it over?

@wucke13 wucke13 force-pushed the master branch 2 times, most recently from 7e92f56 to b9b7192 Compare May 17, 2022 14:50
@wucke13
Copy link
Contributor Author

wucke13 commented May 17, 2022

@emilk Sorry for the long delay. I just applied your (execellent!) suggestion, and rebased. Let's see what the CI says 😄

@wucke13 wucke13 force-pushed the master branch 2 times, most recently from 6b53b6f to 7197fb5 Compare May 21, 2022 12:43
@wucke13
Copy link
Contributor Author

wucke13 commented May 21, 2022

Oof. Vec::retain_mut seems to be a rather new thing. @emilk what do you propose?

@emilk
Copy link
Owner

emilk commented May 23, 2022

In case of a whole update we want to ignore all previous updates, so I suggest simply:

self.delta.set.retain(|(x, _)| x != id); 
self.delta.set.push((id, delta));

introduces a queue for texture changes
@wucke13
Copy link
Contributor Author

wucke13 commented Jun 16, 2022

@emilk Please take a look again :)

epaint/src/textures.rs Outdated Show resolved Hide resolved
@emilk emilk merged commit bd5f553 into emilk:master Jun 19, 2022
Titaniumtown pushed a commit to Titaniumtown/egui that referenced this pull request Jun 20, 2022
Co-authored-by: Wanja Zaeske <wanja.zaeske@dlr.de>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Titaniumtown pushed a commit to Titaniumtown/egui that referenced this pull request Jun 20, 2022
Co-authored-by: Wanja Zaeske <wanja.zaeske@dlr.de>
Co-authored-by: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
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.

Calling set_partial on the same texture rapidly causes some changes to get lost
2 participants