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

Gizmo line joints #12252

Merged
merged 14 commits into from
Mar 11, 2024
Merged

Gizmo line joints #12252

merged 14 commits into from
Mar 11, 2024

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented Mar 2, 2024

Objective

Solution

  • Adds line_joints: GizmoLineJoint to GizmoConfig. Currently the following values are supported:
    • GizmoLineJoint::None: does not draw line joints, same behaviour as previously
    • GizmoLineJoint::Bevel: draws a single triangle between the lines
    • GizmoLineJoint::Miter / 'spiky joints': draws two triangles between the lines extending them until they meet at a (miter) point.
      • NOTE: for very small angles between the lines, which happens frequently in 3d, the miter point will be very far away from the point at which the lines meet.
    • GizmoLineJoint::Round(resolution): Draw a circle arc between the lines. The circle is a triangle fan of resolution triangles.

Changelog

  • Added GizmoLineJoint, use that in GizmoConfig and added necessary pipelines and draw commands.
  • Added a new line_joints.wgsl shader containing three vertex shaders vertex_bevel, vertex_miter and vertex_round as well as a basic fragment shader.

Migration Guide

Any manually created GizmoConfigs must now set the .line_joints field.

Known issues

  • The way we currently create basic closed shapes like rectangles, circles, triangles or really any closed 2d shape means that one of the corners will not be drawn with joints, although that would probably be expected. (see the triangle in the 2d image)
    • This could be somewhat mitigated by introducing line caps or fixed by adding another segment overlapping the first of the strip. (Maybe in a followup PR?)
  • 3d shapes can look 'off' with line joints (especially bevel) because wherever 3 or more lines meet one of them may stick out beyond the joint drawn between the other 2.
    • Adding additional lines so that there is a joint between every line at a corner would fix this but would probably be too computationally expensive.
  • Miter joints are 'unreasonably long' for very small angles between the lines (the angle is the angle between the lines in screen space). This is technically correct but distracting and does not feel right, especially in 3d contexts. I think limiting the length of the miter to the point at which the lines meet might be a good idea.
  • The joints may be drawn with a different gizmo in-between them and their corresponding lines in 2d. Some sort of z-ordering would probably be good here, but I believe this may be out of scope for this PR.

Additional information

Some pretty images :)

Screenshot 2024-03-02 at 04 53 50
  • Note that the top vertex does not have a joint drawn.
Screenshot 2024-03-02 at 05 03 55

Now for a weird video:

Screen.Recording.2024-03-02.at.05.14.23.mov
  • The black lines shooting out from the cube are miter joints that get very long because the lines between which they are drawn are (almost) collinear in screen space.

@TrialDragon TrialDragon added C-Enhancement A new feature A-Gizmos Visual editor and debug gizmos C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 2, 2024
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

This was what I was expecting from this feature. Though I'm not sure about the rendering part, I have a couple of maybe silly questions.

crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/lib.rs Outdated Show resolved Hide resolved
@nicopap nicopap mentioned this pull request Mar 4, 2024
57 tasks
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

The render phase was a bit tricky, but everything right. There is a couple of things in other places that could improve IMO

crates/bevy_gizmos/src/config.rs Outdated Show resolved Hide resolved
crates/bevy_gizmos/src/pipeline_2d.rs Show resolved Hide resolved
crates/bevy_gizmos/src/pipeline_3d.rs Show resolved Hide resolved
Co-authored-by: Pablo Reinhardt <126117294+pablo-lua@users.noreply.github.com>
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

All good, LGTM

@pablo-lua
Copy link
Contributor

The CI error looks like a external error, not related to this PR. I'll have a better look now just to make sure though.

@pablo-lua
Copy link
Contributor

#12254 found the source of the error. I think you have to use bevy_utils for this now.

@alice-i-cecile alice-i-cecile 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 Mar 5, 2024
@alice-i-cecile alice-i-cecile added the C-Needs-Release-Note Work that should be called out in the blog due to impact label Mar 5, 2024
@lynn-lumen lynn-lumen mentioned this pull request Mar 9, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 11, 2024
Merged via the queue into bevyengine:main with commit 27215b7 Mar 11, 2024
27 checks passed
@alice-i-cecile alice-i-cecile removed the C-Needs-Release-Note Work that should be called out in the blog due to impact label Jun 4, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 9, 2024
# Objective

The changes made in #12252
introduced an previously fixed bug in webgpu rendering.

## Solution

This fix is based on #8910 and
applies the same vertex buffer layout assignment for the LineGizmo
Pipeline.

## Testing

- Tested the 3D Gizmo example in webgpu and webgl environments

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
mockersf pushed a commit that referenced this pull request Sep 5, 2024
# Objective

The changes made in #12252
introduced an previously fixed bug in webgpu rendering.

## Solution

This fix is based on #8910 and
applies the same vertex buffer layout assignment for the LineGizmo
Pipeline.

## Testing

- Tested the 3D Gizmo example in webgpu and webgl environments

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature 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.

4 participants