-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 crash when drawing line gizmo with less than 2 vertices #9101
Conversation
It should return success no? |
I'm not sure what the semantics of |
I don't know if the result is correct or not, but it does fix the bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll approve for the sake of getting this merged quickly, but I would prefer if it was returning RenderCommandResult::Success
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RenderCommandResult
are currently ignored
bevy/crates/bevy_render/src/render_phase/draw.rs
Lines 270 to 271 in eb485b1
// TODO: handle/log `RenderCommand` failure | |
C::render(item, view, entity, param, pass); |
But it would make much more sense for me too to return a Success
here.
Also, surprisingly, the solution is not what this PR is actually doing. if you add 3 empty lines, the vertex count will be 3 and not hit that condition, still "rendering" empty lines
adding a gizmo line adds the line position to the list, then unconditionally push an "empty" slot with |
Also I learned recently that we should prefer u32::MAX over NaN. It is the "official" way of telling the GPU that our line list is ending. |
@devil-ira can you make the |
I would prefer if someone tries to fix the issue at the source rather than this PR. We are pushing garbage in the mesh, and this PR just says "oh if there's only one garbage, don't draw it" |
When we add a retained version of the line gizmo well need this check either way as users could easily create a linestrip of length 0 or 1. |
Co-authored-by: François <mockersf@gmail.com>
@mockersf @superdump; I'll defer to one of you on whether we merge this PR as is. |
I'm going to merge this following the approval from @DGriffin91, whose expertise I trust here: crashes are very sever bad and we can revert this very simple fix in a PR with a better fix later. |
@alice-i-cecile imo we should try to get this into 0.11.1 if we're merging it now. I noticed it was removed from the milestone, not sure if that means it would no longer be cherry picked for the point release. |
I'll defer to @cart for that decision, but I do agree. |
Sorry I didn't check my notifications before publishing so I missed this. |
# Objective Fix #9089 ## Solution Don't try to draw lines with less than 2 vertices. These would not be visible either way. --------- Co-authored-by: François <mockersf@gmail.com>
Objective
Fix #9089
Solution
Don't try to draw lines with less than 2 vertices. These would not be visible either way.