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

Prevent segmented primitives from reading outdated clip tasks. #2564

Closed
wants to merge 1 commit into from

Conversation

@nical
Copy link
Collaborator

nical commented Mar 23, 2018

Fixes #2531.

Brush segments store handles to their own clip tasks. If segmentation heuristics change we need to either clear the segment information or update it to ensure the clip tasks they point to are requested (this commit does the latter).


This change is Reviewable

Brush segments strore handles to their own clip tasks. If segmentation heuristics change we need to either clear the segment information or update it to ensure the clip tasks they point to are requested (this commit does the latter).
@nical
Copy link
Collaborator Author

nical commented Mar 23, 2018

r? @glennw

This is I think the simplest way to fix #2531, although I suspect that we might want to instead un-segment primitives in some case to allow the most optimized path when segmentiation heuristics change between frames, or maybe it is enough/correct to go through the segments and clear the clip task ids.
Un-segmenting means we need to differentiate between segments created automatically during frame building and segments created explicitly when flattening the scene, plus probably a bunch of other edge cases I don't know about.
Perhaps we can land the simple fix first and then look for the best solution?

@glennw
Copy link
Member

glennw commented Mar 23, 2018

@nical Nice work finding the cause of this!

I think this patch won't quite do what we want though, because what we'll end up doing is still generating clip tasks for each segment in this case, even though the early outs are saying that we don't need any clip masks at all.

I think if we instead have a loop before those checks which runs through the segment descriptor and clears out the clip tasks, that should fix this bug and also deal with the concern mentioned above?

@nical
Copy link
Collaborator Author

nical commented Mar 23, 2018

Yeah, I know this isn't optimal since we hang on to a heuristic that was good for a previous frame but not the current one. I was unsure whether clearing the render task ids would be enough, since the new frame might not even create segments (in fact in this case I am pretty sure it wouldn't) and so we'd still be in a half-unexpected configuration, but if you think it will do, sounds good to me. I'll give this a go on Monday.

@glennw
Copy link
Member

glennw commented Mar 23, 2018

The segments should always be constant for a given scene, since they rely only on clips in the same reference frame (local space) as the primitive, and therefore shouldn't change with scrolling. So I think it should be fine to just clear the task IDs in this case. Thanks!

@nical
Copy link
Collaborator Author

nical commented Mar 26, 2018

Closing in favor of #2568.

@nical nical closed this Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.