New CSG Polygon bugs introduced by "Fix multiple issues with CSGPolygon #49314"

[jitspoe]

Godot version

3.4 beta 4

System information

Windows 10, GLES3

Issue description

Regarding #49314 I have run into a few issues.

1. Interval is now a fraction of the entire length of the curve (???)* rather than a distance, meaning long curves behave different from short curves with the same settings as far as polygon density goes. This worked better as a distance in meters and was consistent with the curve bake interval. *Not sure if it's actually a fraction, because .1 seems like it would be broken into 10ths, but there are more than 10 segments, and sometimes a value of 1 has more than 1 segment.

2. Can't get the UV's to behave the same as before. The UV behavior is probably better... mostly, but still has oddities.
   Before:
   
   After:
   
   On the plus side, it tries to wrap the texture around. On the minus side, it seems to only use 1/2 of the texture:
   
   I would expect this to wrap around entirely so I could have a tiled pipe texture or something. Maybe it's because the other half of the texture is reserved for the caps? Wonder if that should be an option.
   Also, the texture coverage doesn't take the size of the polygon into account, so a really small face uses the same amount of texture as a really large one:
   
   
   Also, it looks like the UV's reset to 0 for each segment, which works OK for static textures, but not for custom shaders (ex: a pulse that flows across a cable). And, since the segment length is a fraction of the total curve, the overall texture coverage, and thus timing of pulses and other shader effects, don't cover a set distance using the UV's like it used to.

3. Limit of .1 for the interval is insufficient for long curves.
   
   Pretty self-explanatory. I need more detail for my long cables! I actually had an engine modification to simplify based on the curve angle which I need to make a pull request for (after I update it to match the new code).

Steps to reproduce

Make a really long path and try to make the CSG geometry follow it and look good. :)

Minimal reproduction project

test_csg_path_3.4b4.zip
test_csg_path_3.3.3.zip

[Calinou]

cc @madmiraal

[madmiraal]

As mentioned in #49314, there are some compatibility breaking changes that need to be mentioned in the release notes. The changes are by design and were explained and discussed in the original master PR #49313.

1. Interval is now a fraction of the entire length of the curve (???)* rather than a distance

Maybe I'm missing something, but this wasn't changed. It does and always did use fixed steps along the length of the curve using interpolate_baked() method.

2. Can't get the UV's to behave the same as before

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

To prevent the UV mapping from simply repeating on each end and each extrusion (#23257), the UV mapping for the entire CSGPolygon was defined as: 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.

3. Limit of .1 for the interval is insufficient for long curves

A minimum of 0.1 and a step size of 0.05 seemed reasonable for making adjustments in the editor, while keeping the number of polygons at a reasonable number. Since path_interval simply sub-divides each extrusion without adding fidelity, if a user wants to increase the number of extrusions, I expected the preferred approach to be to increase the number of points on the curve and leave path_interval at 1. That been said, this is just a limitation in the editor. 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. If .1 is insufficient for long curves, what would be a reasonable value?

[jitspoe]

Maybe I'm missing something, but this wasn't changed. It does and always did use fixed steps along the length of the curve using interpolate_baked() method.

Old behavior:

This is with interval set to 1. I have a 1m cube for reference. Note that the curve is broken into 1m segments. This is consistent with the bake interval.

New behavior:

Interval is set to 1, but the segments are not 1m in length. There's also more than 1 segment. It seems dependent on the number of control points added to the curve.

The problem with this behavior is that, if I want to have a consistent spacing (ex: every .3m) of my geometry, I have to do a bunch of calculations instead of just setting the interval once. Also, any time I tweak the curve, I have to update these calculations, and even when calculated, it won't be exact,

To prevent the UV mapping from simply repeating on each end and each extrusion (#23257), the UV mapping for the entire CSGPolygon was defined as: 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.

Yeah, that's what I figured. It's probably a good change. The only down side is that you can end up with a seam since the texture won't tile in the middle. Also, could waste texture memory if you don't have the caps visible (ex: pipes). Might be worth adding an option for.

A minimum of 0.1 and a step size of 0.05 seemed reasonable for making adjustments in the editor, while keeping the number of polygons at a reasonable number. Since path_interval simply sub-divides each extrusion without adding fidelity, if a user wants to increase the number of extrusions, I expected the preferred approach to be to increase the number of points on the curve and leave path_interval at 1. That been said, this is just a limitation in the editor. 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. If .1 is insufficient for long curves, what would be a reasonable value?

Since it's based on a subdivision of the curve, it's difficult to say what a good value is. If I have a 100m curve, I might need 0.001, but that would be really small for shorter curves. I now understand why you have the interval set up the way you do (since some people probably want a 1:1 polygon with the number of points added). Perhaps the solution here is to have an interval type: distance vs subdivisions, that way those of us who need to do distance-based things like cables with UVs that repeat at a certain length can use the distance-based one, and people using CSG for other stuff can use that.

Also, I'm using this all in-editor for environment work, so setting those values via code doesn't work well.

[stebulba]

I need to change the interval to minimum of 0.01 and a step of 0.01 for my new pull request of that:
#41499

In this small video, I show the difference between minimum of 0.1 and a step size of 0.05 VS minimum of 0.01 and a step of 0.01
With my pull request the actual setting without code, allow only a low poly style of sculpt.

Interpolation-Path_intervale_Demo.05.vs.01_5fps_video.mp4

Without my new feature, I prefer give us more option with the Editor. 0.1 as minimum is not appropriate when you want modify the angle in the path.

2.3.4-2021-08-31_20.08.38.mp4

[madmiraal]

Interval is set to 1, but the segments are not 1m in length. There's also more than 1 segment. It seems dependent on the number of control points added to the curve.

The problem with this behavior is that, if I want to have a consistent spacing (ex: every .3m) of my geometry, I have to do a bunch of calculations instead of just setting the interval once. Also, any time I tweak the curve, I have to update these calculations, and even when calculated, it won't be exact,

Yes, the calculation for the number of extrusions required was changed to depend on the number of points on the curve and not the length of the curve, which didn't make sense. The number of polygons shouldn't be dependent on the distance. It should be dependent on the number of points on the curve. Increasing the number of polygons without increasing the fidelity of the shape creates unnecessary polygons and vice versa. I notice, however, that the formula is undercounting by 1, so that needs to be fixed.

Ideally, the size of each of the polygons should be dependent on the distance between the curve's points (and path_interval should be inverted to be sub-divisions between the points, so that it's the same as spin_sides in MODE_SPIN). However, the problem is that the UV mapping (with path_continuous set to false) currently also depends on the size of the polygons, and to keep the repeating pattern consistent the polygons need to be of equal length. Hence the compromise of keeping the fixed steps along the length of the curve.

The challenge you're facing is wanting the UV mapping to repeat by distance, which is a different issue. I don't even know how you would do this in Blender. It would, as you suggest, require calculating the repeating interval based on the length of the curve.

If I have a 100m curve, I might need 0.001, but that would be really small for shorter curves.

and

In this small video, I show the difference between minimum of 0.1 and a step size of 0.05 VS minimum of 0.01 and a step of 0.01
With my pull request the actual setting without code, allow only a low poly style of sculpt.

In games you want low polys to reduce the rendering time. Instead smooth shading is used to hide the corners. Note: a setting or 0.01 would create 100 x 2 x polygon.size() faces per extrusion, and a setting of 0.001 would create 1000 x 2 x polygon.size() faces per extrusion! That been said, I accept that 0.1 is too low for curves with large bezier deviations. MODE_SPIN has 64 as a maximum number, which is equivalent to a path_interval of 1/64 ≈ 0.015. So, having a minimum value and step size of 0.01 makes sense.

[jitspoe]

I'm working on a change to address several of the issues I have as well as adding some more flexibility:

1. Option for the interval type (subdivide, like you have, or distance, like it used to use).
2. UV tile distance, so you can set it to 1m, like it used to behave, 0 to cover the whole mesh, like you have, or any custom value you'd like if you want to tile textures at a different distance.
3. Mesh simplification based on customizable angle, so you can have detailed curves, but long near-straight sections won't have tons of subdivisions.
4. Tweaked limits.

I'm trying to keep the changes minimal, but the PoolVector is problematic, because the Write needs to go out of scope before the array can be resized (needs to be resized to reduce the poly count), but that would mean putting a large scope around everything... I don't suppose there's any other way? I know 4.x is going away from the PoolVector. Should we even be using a PoolVector here?

[jitspoe]

I've opened 2 PR's for this in both main and 3.x (since the class name changes, removal of pool vectors, etc. make merging ugly):

3.x: #52509
main: #52512

[BastiaanOlij]

Ok, just to give some more info on this, I know i reacted on madmiraals post awhile ago and just didn't have time to revisit it. I've used CSGPolygon a lot over the past few years and I know various other people who make a lot of use of it as well who will run into trouble with the "wanted breaking changes".

This is a track in 3.3 using path extrusion and most importantly, continious U to create a predictable distance based tiling of the texture over a long distance:

This is that same track in 3.4 (without Jitspoe's fix):

Two things spring out here:

1. Path interval has changed, I need to set this to a much lower value to get the track to render properly again, the end result seems the same though I'm not entirely sure what the unit now represents.
2. But more importantly, continuous U no longer tiles by distance, it seems to stretch the texture over the total track. Yes I can scale U but that isn't a very good solution especially when we are dealing with generated geometry or where use cases like these introduce separate segments with variable lengths (like the pitlane). This is not an acceptable breakage. This use case was the use case for which continuous U was introduced, to create a tileable U over a set and predictable distance and it no longer behaves that way.

[BastiaanOlij]

@jitspoe I just tried out your PR but with that the texture is still stretched over the entire distance instead of re-introducing the original desired behavior

Spoke too soon, helps if I start the correct version of Godot... to be continued... :)

[jitspoe]

What's your UV distance set to? I thought that defaulted to 1, which should have the same behavior as the old CSG (tiles at 1m). (Note: Will most likely be changing this to path_u_distance instead of uv_distance).

[BastiaanOlij]

@jitspoe testing now with the proper build, path interval is working beautifully with interval type set to distance.

UV distance is working but it is now working for both U and V, and that is a problem in my use case because it's nearly impossibly to get the road texture to line up properly with the road. We almost need separate options for U and V.

Love the angle simplify btw, I think I applied a similar approach in my procmesh library, which I'm really starting to feel I should revive.

[BastiaanOlij]

Ok, I don't understand it but purely by trial and error I found out that if I scale my U by 8, the width of my track is exactly right. There probably is a good explanation for this:

[Zireael07]

Separate options for U and V should be a thing either way - I bet there's a ton of use cases that need them.

[BastiaanOlij]

Separate options for U and V should be a thing either way - I bet there's a ton of use cases that need them.

The thing is, you're only extruding along one, so it might just lead to overkill to have too many choices. I think the current state with the changes Jitspoe added will cover the needs of most if not all. I would be against making further changes until someone comes up with a valid use case.

[jitspoe]

UV distance is working but it is now working for both U and V

That's strange. It should only apply along the length of the curve (U). I renamed it to path_u_distance in master since it only applies to the U.

[BastiaanOlij]

UV distance is working but it is now working for both U and V

That's strange. It should only apply along the length of the curve (U). I renamed it to path_u_distance in master since it only applies to the U.

Yup, I drew the wrong conclusion, what you said I think in your reaction on your PR was spot on. I've got 4 sections in my polygon and it's using half of the texture width to make room for the end caps, so each section is 0.125 in length. * 8 makes the V go from 0 to 1.0, it's unaffected by the path_u_distance.