SirRamEsq / SmartShape2D

A 2D Terrain Tool for Godot
MIT License
1.34k stars 67 forks source link

Improvements to edge generation #156

Closed Technostalgic closed 4 months ago

Technostalgic commented 4 months ago

This is an amazing plugin with an incredible and impressive scope, unfortunately however, while using this plugin I encountered several deal breaker issues with edge mesh generation around corners and rendering artifacts from unnecessary texture tiling. Here are some improvements to the mesh generation for shape edges which attempt to remedy the issues:

Sharp Corner Tapering

taper_sharp_corners
This feature adds an option on the edge material metadata resource ("taper_sharp_corners") which allows the user to enable or disable the feature. When enabled, any corners sharper than a right angle (unless there is a corner there) will not be welded if welding is enabled, and the ends of them will be tapered in the same way that the edges are tapered with the "use_taper_texture" property in the edge material. This removes the ugly distortion that appears when welding sharp corners. This feature will only work if the edge material has and uses taper textures.

Corner Anti-Distortion

corner_anti_distortion
This improvement reworks the build_quad_corner method in the shape.gd script to more accurately align with the edges of the shape. Previously, the corners were distorted in a way that worked against the edge alignment, causing them to appear increasingly misaligned the further away from a right angle it became.

Remove Tiling Artifacts

tiling_artifacts
When the corner textures and taper textures were rendered with bilinear filtering, they would leave behind rendering artifacts due to the start of the image interpolating the in-between-pixel values of the starting image pixel wrapped around to the ending image pixel. To fix this, I identified which quads were being used to generate meshes with corner or taper textures, and when creating render nodes for them, I disable tiling for those nodes.

Admittedly, I've only been using this plugin for a few days and do not have solid grasp on the codebase just yet. Through my testing I was not able to discover any bugs that my changes introduced, however it is possible that I have missed some. That being said, there may be some room for optomization here. If you have questions or would like me to do any revisions, I will be happy to work with you.

Thanks, Isaiah

Technostalgic commented 4 months ago

sounds good! I should be able to resolve these issues soon

limbonaut commented 4 months ago

PR looks good! I found one issue with sharp corner tapering, and I'm not sure if it's introduced by this PR, or it's something else. Illustration of the issue: image

Test scene used: test_mirrored.zip

Technostalgic commented 4 months ago

ha, my bad. thought I caught all those little issues. Will be fixing them momentarily 😅 still getting used to gdscript

I'll look into the issue mentioned here https://github.com/SirRamEsq/SmartShape2D/pull/156#issuecomment-2169189238

Technostalgic commented 4 months ago

After looking more into the issue referenced above, here: https://github.com/SirRamEsq/SmartShape2D/pull/156#issuecomment-2169189238

It looks like it is indirectly caused by the changes I made, as I'm using the taper_quad function in situations it would not have normally been used. However, the function itself, I didn't actually write. It was an inline part of the build_edge_with_material function here: https://github.com/SirRamEsq/SmartShape2D/blob/c4d8169cc996a47388fe40140f71ee61483f9457/addons/rmsmartshape/shapes/shape.gd#L1678-L1716 which I had to re-use so I moved it into it's own function declaration.

There are some parts of the function I don't quite understand what is going on (namely the part where the offset variable is calculated, and what it does, or how it's used) and while I think it is possible that is the part that could be causing the issue, I don't actually think this is an issue that will pop up in any real applications of the feature.

This is because the issue only seems to occur if and only if the taper left and right textures reference the same texture resource. To test this, you can take the example scene test_mirrored uploaded by @limbonaut, open it in the project, duplicate the icon.png texture, and set that new duplicated textures as either of the two taper textures in the edge material and then it will render properly.

If, for whatever reason, someone is using the same texture for left and right taper textures, they can disable the taper_sharp_corners option and it will behave as expected. But I suspect it will take me a significant amount of time and resources to try and fix this issue, which I'm not willing to devote at this time since I have no idea what exactly causes it or how to fix it

mphe commented 4 months ago

Thank you for looking into this! I created a new issue for this bug #158.

limbonaut commented 4 months ago

I looked at the corner anti-distortion code, and it works well, but it seems to me it's trying to compensate for something we are calculating wrong in the first place. I need a little bit more time to figure this out.

limbonaut commented 4 months ago

@mphe @SirRamEsq @Technostalgic Should we actually couple "sharp corner" tapers with "normal end" tapers? It seems to me, it may be useful to have both and with different textures. Especially, if more than one edge material is used.

mphe commented 4 months ago

Agree, more configurability is usually better.

limbonaut commented 4 months ago

I looked at the corner anti-distortion code, and it works well, but it seems to me it's trying to compensate for something we are calculating wrong in the first place. I need a little bit more time to figure this out.

I'll fix it in a separate PR.

Technostalgic commented 4 months ago

@mphe @SirRamEsq @Technostalgic Should we actually couple "sharp corner" tapers with "normal end" tapers? It seems to me, it may be useful to have both and with different textures. Especially, if more than one edge material is used.

Sorry, I'm not sure what you mean by this? Couple them together how?

mphe commented 4 months ago

At the moment corners tapers and sharp corners use the same texture. @limbonaut suggested having separate texture settings for them.

limbonaut commented 4 months ago

Sorry, I'm not sure what you mean by this? Couple them together how?

I mean that in this PR, when taper_sharp_corners is enabled, the same set of textures is used for edge endings and also for sharp corners. And the question is, whether we should have a separate set of textures (separate material property) for sharp corners instead of reusing tapers?

Technostalgic commented 4 months ago

Oh! yeah I think that's probably a better idea. My project has no need for taper textures so I think I just decided to utilize them this way since I knew I wouldn't be needing them. It should be easy enough to add a new texture field and use that instead though, I can go ahead and do that

limbonaut commented 4 months ago

It should be easy enough to add a new texture field and use that instead though, I can go ahead and do that

I think, we have a consensus then :+1:
And I'll look through the rest of the PR (didn't check it all yet).

Technostalgic commented 4 months ago

Separate sharp corner taper textures are implemented and ready to go once changes are approved 👍

Technostalgic commented 4 months ago

Just added one final tweak that I forgot about before, there's no need for sharp corner tapering to require the "use_taper_textures" property any more in the edge material since it has it's own texture fields now

mphe commented 4 months ago

LGTM. Thanks for contributing and making this plugin better!