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 crash when drawing line gizmo with less than 2 vertices #9101

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

tim-blackbird
Copy link
Contributor

Objective

Fix #9089

Solution

Don't try to draw lines with less than 2 vertices. These would not be visible either way.

@nicopap
Copy link
Contributor

nicopap commented Jul 10, 2023

It should return success no?

@tim-blackbird
Copy link
Contributor Author

I'm not sure what the semantics of RenderCommandResult are intended to be.
The RenderCommandResult doesn't appear to be used anywhere.

@Selene-Amanita Selene-Amanita added C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash A-Gizmos Visual editor and debug gizmos labels Jul 10, 2023
@Elabajaba
Copy link
Contributor

I don't know if the result is correct or not, but it does fix the bug.

@nicopap nicopap added this to the 0.11.1 milestone Jul 18, 2023
Copy link
Contributor

@nicopap nicopap left a 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

@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jul 18, 2023
Copy link
Member

@mockersf mockersf left a 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

// 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

@mockersf
Copy link
Member

adding a gizmo line adds the line position to the list, then unconditionally push an "empty" slot with NAN. could we instead not push those nans when we didn't push actual data to the lines?

@nicopap
Copy link
Contributor

nicopap commented Jul 22, 2023

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.

@alice-i-cecile
Copy link
Member

@devil-ira can you make the RenderCommandResult change requested and then I'll merge this in for you.

@mockersf
Copy link
Member

@devil-ira can you make the RenderCommandResult change requested and then I'll merge this in for you.

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"

@tim-blackbird
Copy link
Contributor Author

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.

@alice-i-cecile
Copy link
Member

@mockersf @superdump; I'll defer to one of you on whether we merge this PR as is.

@cart cart removed this from the 0.11.1 milestone Aug 9, 2023
@alice-i-cecile
Copy link
Member

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.

@DGriffin91
Copy link
Contributor

@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.

@alice-i-cecile
Copy link
Member

I'll defer to @cart for that decision, but I do agree.

@cart
Copy link
Member

cart commented Aug 15, 2023

Sorry I didn't check my notifications before publishing so I missed this.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 15, 2023
Merged via the queue into bevyengine:main with commit cfa3303 Aug 15, 2023
20 checks passed
@cart cart added this to the 0.11.2 milestone Aug 16, 2023
cart pushed a commit that referenced this pull request Aug 17, 2023
# 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 <[email protected]>
@tim-blackbird tim-blackbird deleted the gizmo-troubles branch August 17, 2023 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Gizmos Visual editor and debug gizmos C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attempting to render empty linestrips causes a crash
9 participants