Skip to content

Conversation

@vaner-org
Copy link
Contributor

@vaner-org vaner-org commented Sep 1, 2025

Closes #12550.

Adds an editor setting 3D > Ignore Backfaces on Selection that is true by default.
Adds an optional parameter and an if statement to TriangleMesh's intersect_ray function.

@vaner-org vaner-org requested review from a team as code owners September 1, 2025 23:59
@YeldhamDev
Copy link
Member

This should also be implemented for remote selection.

@vaner-org vaner-org force-pushed the selection-ignore-backfaces branch from d388708 to 746aa1d Compare September 2, 2025 00:13
@vaner-org vaner-org requested a review from a team as a code owner September 2, 2025 00:13
@vaner-org
Copy link
Contributor Author

This should also be implemented for remote selection.

✔️ Done.

@vaner-org vaner-org force-pushed the selection-ignore-backfaces branch from d43059d to 4325e1f Compare September 2, 2025 08:20
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 on https://github.com/Calinou/godot-reflection, it works as expected. Code looks good to me.

In the example below, the camera is stuck inside geometry.

Before

ignore_backfaces_before.mp4

After

ignore_backfaces_after.mp4

@KoBeWi
Copy link
Member

KoBeWi commented Sep 2, 2025

Does it need to be a setting? Why would someone want to turn it off?

@Calinou
Copy link
Member

Calinou commented Sep 2, 2025

Does it need to be a setting? Why would someone want to turn it off?

I can't see a reason to turn it off, so I think the setting should be removed too. We can introduce it in a future PR if there's a proven need for it.

@vaner-org vaner-org force-pushed the selection-ignore-backfaces branch from 4325e1f to 93195f5 Compare September 3, 2025 00:47
@vaner-org
Copy link
Contributor Author

I've removed the editor setting and hardcoded our two calls to intersect_ray to ignore backfaces.

@Repiteo Repiteo modified the milestones: 4.x, 4.6 Sep 10, 2025
@kleonc
Copy link
Member

kleonc commented Oct 1, 2025

Note that the cull mode the given mesh is rendered with is not being accounted for anyhow (it's the same before/after this PR though).

So while this PR fixes the behavior for the CULL_BACK/back case from the tables below, it breaks CULL_FRONT/back and CULL_DISABLED/back cases.
If CULL_BACK/back case is more common/annoying than CULL_FRONT/back and CULL_DISABLED/back cases together, then this PR could be considered as an improvement even in it's current state. Whether this is actually the case I have no idea.

Without this PR (v4.5.stable.official [876b290]):

Cull mode mesh faces looked at from visible selectable correct
CULL_FRONT front
CULL_FRONT back
CULL_BACK front
CULL_BACK back
CULL_DISABLED front
CULL_DISABLED back

With this PR (v4.5.beta.gh-110185 [4d10a65]):

Cull mode mesh faces looked at from visible selectable correct
CULL_FRONT front
CULL_FRONT back
CULL_BACK front
CULL_BACK back
CULL_DISABLED front
CULL_DISABLED back

E.g. selection for double sided Sprite3D (by default double_sided = true; it uses CULL_DISABLED mode internally) gets broken with this PR as it makes the sprite selectable only from the front side.

Before:

Godot_v4.5-stable_win64_NrlaBoyJEj.mp4

After:

godot.windows.editor.x86_64_eufXVIyjSr.mp4

@ratkingsminion
Copy link

In our case the PR would be an improvement, we have levels full of 3D models (each wall, floor, ceiling is a mesh on its own) and none of them have front face culling. I think that will be the case for the majority of users, at least I can't imagine otherwise. We use Sprite3Ds mostly as billboards, and I guess the PR does not account for that either (it's already a problem in the current way of selecting).

@vaner-org vaner-org closed this Oct 18, 2025
@vaner-org vaner-org reopened this Oct 18, 2025
@vaner-org
Copy link
Contributor Author

vaner-org commented Oct 19, 2025

Addressing @kleonc's concerns, I've added an extra commit on which I'd appreciate some feedback.
Assuming the precedence used in MeshInstance3D's get_active_material(), for each case, from highest to lowest priority in how the cull mode is inferred:

MeshInstance3D Sprite3D CSGPrimitive3D
GeometryInstance3D material override Geometry3DInstance material_override Geometry3DInstance material_override
MeshInstance3D surface (0) override SpriteBase3D double_sided CSGShape's specific get_material
MeshInstance3D mesh material

This means that the new parameter accepted by intersect_ray is now an enum, one of CULL_BACK, CULL_FRONT and CULL_DISABLED.

All of this can be tested in the scene here: backface_test.zip

Here's the problem:
It seriously hurts editor performance, adding a noticeable delay when selecting nodes. If anyone here is aware of a more performant way of doing this, please let me know.

Alternatively, I can revert this commit, retaining the bandaid fix for Sprite3D, and then branch off the remaining functionality into another draft PR, where an optimised way of doing this can be discussed, and the fix for Sprite3D can be replaced with something that can represent all mesh planes.

@vaner-org vaner-org force-pushed the selection-ignore-backfaces branch from 8677866 to aa78e68 Compare October 19, 2025 02:26
@vaner-org vaner-org force-pushed the selection-ignore-backfaces branch from aa78e68 to 34e1cf1 Compare October 19, 2025 02:28
@Repiteo Repiteo requested review from Calinou and kleonc October 28, 2025 16:53
@Repiteo
Copy link
Contributor

Repiteo commented Oct 28, 2025

@Calinou @kleonc What's the call then? I don't think we should integrate something that hurts editor performance, so this is more about if we should allow the standalone version to exist with known issues or if a fully optimized implementation is necessary to start

@Calinou
Copy link
Member

Calinou commented Oct 28, 2025

What's the call then? I don't think we should integrate something that hurts editor performance, so this is more about if we should allow the standalone version to exist with known issues or if a fully optimized implementation is necessary to start

Indeed, this will introduce usability issues in large projects even if they didn't run into any issues previously. In this sense, it would be a regression.

I think we should hold off on merging this until we find a more optimized approach. Maybe reusing the depth buffer is something we should do eventually...

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.

Ignore backfaces when selecting Node3Ds in the Editor viewport

8 participants