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

Add VisualShape2D node for drawing common shapes #99210

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kitbdev
Copy link
Contributor

@kitbdev kitbdev commented Nov 14, 2024

Screenshot 2024-11-13 193704

Draw common shapes like Rectangles, Circles, Equilateral Triangles, Right Triangles, Capsules, and Regular Polygons.
Vector-based like Polygon2D, does not rely on textures.
Outline with variable thickness.
Antialiasing.
Solid color (the above image uses 2D lighting on some shapes).
UVs (except on outlines and antialiasing) for shader support.
Regular polygons are under the Circle shape by changing the resolution.

In the editor:
Convert to MeshInstance2D or Polygon2D.
Add CollisionShape2D or ShadowMask2D sibling.
These are similar to the functionality of Sprite.

image

Implementation notes:
  • The name Shape2D is taken so I went for VisualShape2D instead.
  • I couldn't use Shape2D resources since it has Shapes that don't really make sense for drawing like separation ray or world boundary. It also didn't have all the shapes I want, I didn't want to add new ones since it affects physics workflow, and I didn't want to just hide them under convex shape somehow. Also, I wanted to not use a resource, to be more beginner friendly when duplicating nodes.
  • I didn't add a Texture option to try to keep things simple, if a texture is wanted, a Sprite and clip children can be used, or a custom material.
  • The canvas editor rect tool adjusts the size instead of the scale. Using a size variable is important for easier nesting of shapes with different sizes, for the capsule shape, and for antialiasing.
  • Capsules are vertical only, with some more math it could probably dynamically be vertical or horizontal based on the size.
  • This can add CollisionShape2Ds instead of CollisionPolygon2D nodes compared to Sprite2D.
  • Ovals (Circle shape with non-uniform size) will generate a circle collision shape rather than a convex one, since I didn't want it to decide which one it should do automatically. Right now they use the max, so the circle covers the entire oval.
  • Outlines just use the draw method's implementation, so it has limitations. It doesn't have proper UVs and it can look off at large sizes on some shapes. When converting or adding sibling, the outline will be ignored. I also ignore the outline for the editor tools since calculating the size isn't as straightforward as just adding the outline width, especially when it gets larger on non-uniform sized shapes. Also thin outlines may be hard to click on.
  • Enabling outlines will stop rendering the interior of the shape, since rather than having an outline color property, a second shape could be used to fill it.
  • Antialiasing works by adding a 1px outline, since the draw_polyline function has it and draw_colored_polygon doesn't. This doesn't have UVs, and half of the line is drawn behind the shape. Alias is also affect by scaling.
  • The resolution property allows the circle shape to become any regular polygon. Maybe circles and regular polygons should be separated with different default resolutions? I kept equilateral triangle separate even though it can be made with a circle as it is common and the circle shape triangle is offset from the base, which makes it a bit harder to work with.
  • The default resolution is 64 since that is what draw_circle uses.
  • Polygon2D UV data is weird, it isn't in the range 0-1 like other UV data and it seems to only apply if a texture is set, even if a material would make use of it. When generating the UVs for it I use the size of the shape, so a 128x128 texture fits a 128x128 shape.
  • I added some keywords, but there might be other appropriate ones I missed.

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Oh wow, I was not expecting this to be requested this much.

When adding new nodes like this, my only personal concern is compiled binary size.

@Mickeon
Copy link
Contributor

Mickeon commented Nov 17, 2024

Despite the current milestone and popular demand, this feature would require a good chunk of more recent discussion. Merging for 4.4 is not as straightforward as one may think. As is the general rule of thumb, I'm going to shift this PR's milestone to 4.x.

Bear in mind this does not mean the PR is abandoned, or undesired. If maintainers approve of it as is, it'll be merged and the milestone changed.

@Mickeon Mickeon modified the milestones: 4.4, 4.x Nov 17, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Nov 17, 2024

For reference:
Previous implementation: #16483 (rejected)
Alternative: #84587

I tested this and I like the implementation a lot. It's complete with its own editor and the shapes even work properly with shaders. The rectangle shape is a Node2D equivalent of ColorRect, so I imagine it can be used outside prototyping (previous workaround was a scaled Sprite2D with 1px texture).

There was lots of discussion in the proposal and the alternative of simply using CollisionPolygon2D or custom-drawn Shape2D is much inferior to this PR's implementation. Also this can't be easily replicated with a plugin (it's a lot of code); it could work nice as a GDExtension though.

I said I like the implementation, but that's not really enough to merge a feature. As a heavy 2D user, I could maybe see myself using it in some cases, but the existing workarounds are good enough. However I'm not against merging this either.

I didn't add a Texture option to try to keep things simple, if a texture is wanted, a Sprite and clip children can be used, or a custom material.

It's pretty much given that a texture will be among the first things requested and someone will eventually add it. Every time engine gets a new thing, it's followed by various follow-up feature requests (something I observed recently).


Playing with resolution gives interesting results 🤔
image

I think the capsule should change orientation when width is bigger than height.

@kitbdev
Copy link
Contributor Author

kitbdev commented Nov 18, 2024

Yeah, I figured I'd give it another try since the proposal seems wanted and has the implementer wanted tag.

For the Texture support, maybe we can add a note in the documentation to head that off? With texture support comes wanting UV offsets, flipping, atlas and animation support, and at that point it would be better to just inherit from Sprite2D.

Added horizontal capsules. I used an approach that makes sure the shape will be the same as the vertical one, if rotated.
However if the resolution is small and not a multiple of 4, the transition from horizontal to vertical may be noticeable. The alternative is to have the shapes not be identical when rotated, I can change to that approach if wanted.
Also some resolutions may look a bit different than in the previous commit, due to changing the starting point, they no longer always have a point at the ends, and the width should be more consistent.
Making a CollisionShape2D based on a horizontal capsule will automatically rotate it so it matches.

image

Also fixed issue when changing the rect in 2D editor when offset was not at the center. Offset now moves to be in the same relative position.

@KoBeWi
Copy link
Member

KoBeWi commented Nov 18, 2024

Yeah, I figured I'd give it another try since the proposal seems wanted and has the implementer wanted tag.

Indeed, seems like it was basically accepted: godotengine/godot-proposals#1126 (comment)
I missed this comment.

collision_shape_2d_instance->set_shape(shape);
collision_shape_2d_instance->translate(visual_shape_2d->get_offset());
} break;
case VisualShape2D::SHAPE_CIRCLE: {
Copy link
Member

Choose a reason for hiding this comment

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

This might need a special case for ovals. It's the only shape that doesn't get accurate collider.

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 could calculate the eccentricity and make a ConvexPolygonShape2D in some cases, but I didn't want to arbitrarily choose some threshold. I also want to have it use CircleShape2D where possible, since it is better for physics.
I can play around with thresholds and see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eccentricity wasn't very helpful, for now I went with a 10% difference in size.
If it still feels off, I could instead do what MeshInstance3DEditor does and have a menu and let the user choose whether to make it a shape or a convex polygon. This would also allow for small resolution shapes (like hexagon) to have a polygon option.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

@akien-mga
Copy link
Member

I brought this PR to the attention of more maintainers, which sparked some discussion on how we might want to go one step further and fully implement 2D CSG. @smix8 already had a go at it: #99911.

Could you help review that and assess if that fulfills the same use case as this PR in a better way?

@Calinou
Copy link
Member

Calinou commented Dec 5, 2024

Could you help review that and assess if that fulfills the same use case as this PR in a better way?

I feel 2D CSG is best kept separate, as even if you use a single CSG2D node with no children, it'll still have more overhead than a VisualShape2D node. The same applies in 3D already.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Some feedback:

  • The Circle mode doesn't extend the polygon all the way down with odd side counts (this happens regardless of the node's aspect ratio):
visualshape2d_circle.mp4

This also happens with the Capsule mode with certain side counts such as 5, albeit in a less extreme manner.

  • The Capsule mode has identical looks with some side counts (such as 5 and 6). I'd expect the number of sides specified to be present on each cap of the shape, instead of being the sum of both top and bottom caps. For reference, this is how it's specified in CapsuleMesh in 3D.
visualshape2d_capsule.mp4
  • The Outline Width property disables filled shape drawing whenever its value is greater than 0.0. This means you can't draw a filled shape with an outline with a single node, which is inconsistent with how many other nodes work (such as Label or Label3D).

    • Instead, I suggest adding separate Filled (bool), Color and Outline Color properties, and have the Outline Width property control outline thickness drawing without disabling filled shape drawing.
  • Outline Width could allow a value of -1.0 to draw pixel-perfect lines that don't scale with viewport zoom, although antialiasing wouldn't be supported this way. This behavior would essentially be opposite of CanvasItem.draw_line() where -1.0 draws a 1-pixel thick line that scales with viewport zoom. (For a higher-level API, I feel 1.0 should scale with viewport zoom instead, as it's more likely to match users' expectations.)

  • The antialiasing feather doesn't scale with viewport zoom, and there's no way to override its size (which can be used to create blurred shapes). The CanvasItem drawing functions don't expose a way to do this, so we'll have to leave it as-is for now. This could be worth exploring in its own proposal.

  • There could be an extra mode to draw a rectangle with rounded corners (similar to StyleBoxFlat but with a Node2D base), but it can be done later.

@kitbdev
Copy link
Contributor Author

kitbdev commented Dec 13, 2024

Thanks for the feedback!

The Circle mode doesn't extend the polygon all the way down with odd side counts (this happens regardless of the node's aspect ratio):

These are regular polygons, so the points are an equal distance from the center, so I can't really stretch them to reach the bottom. They won't always fit into a square shape due to the math.
Maybe I can reduce the visual rect height for it? But then it wouldn't match the size property and it would be more complex to set.

The Capsule mode has identical looks with some side counts (such as 5 and 6). I'd expect the number of sides specified to be present on each cap of the shape, instead of being the sum of both top and bottom caps. For reference, this is how it's specified in CapsuleMesh in 3D.

The problem with this is that the capsule's resolution and the circle's resolution are the same, so I wanted it to act similarly. If it controlled the capsule's cap size, then the default value of 64 would have twice as many vertices as the circle does.
I can do it, though maybe I should halve it when switching to / from the capsule shape like I do with the width?

Instead, I suggest adding separate Filled (bool), Color and Outline Color properties, and have the Outline Width property control outline thickness drawing without disabling filled shape drawing.

I wanted to reduce the amount of properties, but I can do this.
Should the outline be in front or behind the shape? If it is in front, it matches how it looks now with 2 nodes, and the antialiasing would affect the inner edge. If it is in the back antialiasing would not affect the inner edge, but it would better match Label.

Outline Width could allow a value of -1.0 to draw pixel-perfect lines that don't scale with viewport zoom, although antialiasing wouldn't be supported this way. This behavior would essentially be opposite of CanvasItem.draw_line() where -1.0 draws a 1-pixel thick line that scales with viewport zoom. (For a higher-level API, I feel 1.0 should scale with viewport zoom instead, as it's more likely to match users' expectations.)

I can add this.
I'm not sure about inverting the behavior from the draw_line methods. If it is negative at all it draws a 1px line that doesn't scale. If I flipped it, then to have a scaling width it would need to be negative, and changing the positive values wouldn't make a difference. Maybe it could be a bool in the inspector instead unscaled outline or something and it would hide the outline width property.

There could be an extra mode to draw a rectangle with rounded corners (similar to StyleBoxFlat but with a Node2D base), but it can be done later.

This would require more parameters for each corner radius to match the StyleBoxFlat, and since this isn't resource-based, it would have to be on the base and then hidden on all other shapes (like the resolution).

@Calinou
Copy link
Member

Calinou commented Dec 13, 2024

Should the outline be in front or behind the shape? If it is in front, it matches how it looks now with 2 nodes, and the antialiasing would affect the inner edge. If it is in the back antialiasing would not affect the inner edge, but it would better match Label.

I'd put the outline in front of the shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a Shape2D node to draw 2D shapes
8 participants