Fix multiple issues with CSGPolygon #49313

madmiraal · 2021-06-04T16:29:14Z

Currently, there are a number of issues with the way face vertices and UV mappings are created in CSGPolygon. This results in normals being inverted, UVs repeating when they shouldn't, and extrusions rotating when they shouldn't. This makes CSGPolygon practically unusable. Furthermore, each CSGPolygon extrusion mode has its own code, which not only results in a lot of duplicated code, but requires fixes to be applied in multiple places or issues occurring in one mode and not another. This PR fixes the issues and consolidates the code to provide a standard way of creating CSGPolygons.

To prevent the UV mapping from simply repeating on each end and each extrusion, I've taken the liberty of defining a UV mapping for the entire CSGPolygon: the top half is rolled around the extrusions (u along the the length of the extrusions and v around the outline of the 2D polygon shape) and the bottom half left and right sides are used for the front and end caps respectively.

So, for example this texture:

Maps to a sample CSGPolygon in MODE_SPIN, 270° like this:

In MODE_PATH, the path_continuous_u parameter (which now defaults to true to make it consistent with the other extrusion modes) can be set to false to create a repeating pattern along the length of the path as before:

`path_continuous_u` = `true`	`path_continuous_u` = `false`

MODE_PATH's Path Rotation modes have been fixed so they do what I believe they were intended to do:

PATH_ROTATION_POLYGON: The polygon shape is not rotated.
PATH_ROTATION_PATH: The polygon shape is rotated along the path, but it is not rotated around the path axis.
PATH_ROTATION_PATH_FOLLOW: The polygon shape follows the path and it's rotations around the path axis.

In addition to making MODE_PATH's path_continuous_u parameter default to true, MODE_PATH's Path Rotation mode now defaults to PathFollow, because this is the mode most likely to provide a viable shape from a Path3D, and MODE_PATH's Path Interval property is now constrained to (0; 1].

Finally, the documentation has been updated to reflect both these changes and make some of the other settings clearer.

Closes #23257
Fixes #48233
Fixes #48408
Supersedes #48412

stebulba · 2021-06-04T17:35:22Z

The code is clearer, I like it.
But my pull request have to rewrite : #41499

madmiraal · 2021-06-04T17:57:17Z

@stebulba I haven't looked at #41499, but hopefully these changes will actually make it easier to implement godotengine/godot-proposals#1062 for all modes.

stebulba · 2021-07-05T20:41:13Z

modules/csg/csg_shape.cpp

 	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "spin_degrees", PROPERTY_HINT_RANGE, "1,360,0.1"), "set_spin_degrees", "get_spin_degrees");
 	ADD_PROPERTY(PropertyInfo(Variant::INT, "spin_sides", PROPERTY_HINT_RANGE, "3,64,1"), "set_spin_sides", "get_spin_sides");
 	ADD_PROPERTY(PropertyInfo(Variant::NODE_PATH, "path_node", PROPERTY_HINT_NODE_PATH_VALID_TYPES, "Path3D"), "set_path_node", "get_path_node");
-	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "path_interval", PROPERTY_HINT_EXP_RANGE, "0.001,1000.0,0.001,or_greater"), "set_path_interval", "get_path_interval");


Why did you change the range of path_interval from "0.001,1000.0,0.001,or_greater" to "0.1,1.0,0.1" ?
For exemple In my slide of my proposal I am using around "0.145".
at least should be "0.01,1.0,0.01" but in same time that limit us a lot in possibility.
Maybe can be "( 0.005 or 0.01 ),1.0,0.001" to save us a lot of process if we set path_interval to "0.001" accidentally.

@stebulba I assume you agree that it should never need to be greater than 1. Therefore, the question is about the minimum and the step size.

What is your use case for having a smaller minimum and/or smaller step sizes? Since path_interval simply sub-divides each extrusion without adding fidelity. If a user wants to increase the number of extrusions, I would expect the preferred approach to be to increase the number of points on the curve and leave path_interval at 1.

It's also worth pointing out that this is just a limitation in the editor. A minimum of 0.1 and a step size of 0.1 seemed reasonable for making adjustments in the editor. However, the user can always set the path interval to any value between (0:1] in code using set_path_interval() or the property path_interval.

I understand.

I used path_interval 0.152 and 360 points to do the 360 degrees.

For approximately the same result path_interval 1 and 60 points to do the 360 degrees.
And the render I a lot better. This point is ok.

Edit [no bug found anymore]

BastiaanOlij · 2021-07-06T00:05:09Z

I'm not sure I get the change for path_continuous_u, this was introduced to have the U follow the length of the extruded path. So it would be 0 at the start, and equal to the length of the path at the end. This allows using a tiled texture over the length of the path. A use case here is using CSGPolygon to extrude a road for a driving game where there is a nice repeating road texture.

It sounds like you're changing that behavior which is going to break peoples games.

I do agree that the UV mapping in CSGPolygon leaves a lot to be desired.

madmiraal · 2021-07-07T09:41:24Z

I'm not sure I get the change for path_continuous_u

The only change made to path_continuous_u is the default. It now defaults to true. This makes it consistent with the other extrusion modes. Setting it to false still results in the old behaviour of creating a repeating pattern along the length of the path.

stebulba · 2021-08-11T06:52:30Z

I found a bug. When you reverse the polygon like a mirror or you reverse one point, the mesh is not created as the previous version. This bug, broke my game.
What this change do :

bug_csg_3.X_after.change.mp4

What should do before your change :

no-bug_csg_3.3.2_stable_before.change.mp4

In my game this bug force me to invert face when this pull suppose to fixe it and the front and the back face is inverted with the sides faces.

madmiraal · 2021-08-11T16:23:54Z

Fixed bug identified by @stebulba above, and increased the granularity of path_interval in the editor to 0.05 to allow values like 0.25.

modules/csg/csg_shape.cpp

modules/csg/doc_classes/CSGPolygon3D.xml

akien-mga

I'm not familiar with that code but from a quick look it seems sensible, and it fixes important issues.

madmiraal · 2021-08-12T08:56:09Z

Updated with suggested changes to the documentation and the error message.

akien-mga · 2021-08-12T09:40:29Z

Thanks!

aaronfranke · 2021-08-16T03:05:28Z

modules/csg/csg_shape.cpp

@@ -2228,11 +2053,11 @@ void CSGPolygon3D::_bind_methods() {

 	ADD_PROPERTY(PropertyInfo(Variant::PACKED_VECTOR2_ARRAY, "polygon"), "set_polygon", "get_polygon");
 	ADD_PROPERTY(PropertyInfo(Variant::INT, "mode", PROPERTY_HINT_ENUM, "Depth,Spin,Path"), "set_mode", "get_mode");
-	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "depth", PROPERTY_HINT_RANGE, "0.001,1000.0,0.001,or_greater,exp"), "set_depth", "get_depth");
+	ADD_PROPERTY(PropertyInfo(Variant::FLOAT, "depth", PROPERTY_HINT_RANGE, "0.01,100.0,0.01,or_greater,exp"), "set_depth", "get_depth");


Was there any particular reason to make this less granular, raise the minimum, and lower the maximum?

madmiraal added bug enhancement topic:core topic:3d labels Jun 4, 2021

madmiraal added this to the 4.0 milestone Jun 4, 2021

madmiraal requested a review from a team as a code owner June 4, 2021 16:29

madmiraal mentioned this pull request Jun 4, 2021

[3.x] Fix multiple issues with CSGPolygon #49314

Merged

madmiraal force-pushed the fix-48408 branch from 4d0e54f to da49d61 Compare June 4, 2021 16:33

stebulba mentioned this pull request Jul 5, 2021

CSGPolygon Add interpolation (Scaling/Moving) in mode Path #41499

Closed

stebulba reviewed Jul 5, 2021

View reviewed changes

madmiraal force-pushed the fix-48408 branch from da49d61 to 653b895 Compare August 11, 2021 16:19