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

Extrusion produces wrong indexing -> visual artifacts #356

Open
martinRenou opened this issue Jun 3, 2024 · 17 comments
Open

Extrusion produces wrong indexing -> visual artifacts #356

martinRenou opened this issue Jun 3, 2024 · 17 comments

Comments

@martinRenou
Copy link
Member

Similar to #335 but for extrusion result

Screenshot from 2024-06-03 10-50-00

@martinRenou martinRenou added the bug label Jun 3, 2024
@trungleduc
Copy link
Member

trungleduc commented Jun 3, 2024

hmm, could you test it with a previous version, maybe before #353 ?

@trungleduc trungleduc added this to the 2.0.0 milestone Jun 7, 2024
@martinRenou
Copy link
Member Author

hmm, could you test it with a previous version, maybe before #353 ?

I also reproduce the same-ish issue before that PR

@martinRenou
Copy link
Member Author

It looks like it was working in 2.0.0a0 and stopped working in 2.0.0a1

@trungleduc
Copy link
Member

trungleduc commented Jun 25, 2024

wow, there are only 3 commits between a0 and a1, and two are meaningless. The last one does not touch the rendering part

jcad

@martinRenou
Copy link
Member Author

It doesn't make any sense right.

Also we hard pin the open-cascade build so nothing could have changed there.

@trungleduc
Copy link
Member

actually, we are wrongly pinning the jupytercad-* package in the meta package. Installing jupytercad always gets the latest jupytercad-* ones. This issue happens between a2 and a4, which contains the 3D refactoring pass. I will take a look at it

@martinRenou
Copy link
Member Author

Good catch!

@trungleduc
Copy link
Member

trungleduc commented Jun 25, 2024

I managed to track it down into 2 issues:

jcad0

It looks like these faces are related to the clipping panel, they are introduced in #333

  • if I remove it, I get a better one

jcad4

it still has the face normal points to the inside, but I'm not sure if it's an issue or it's the behavior of OpenCascade, since if I extrude in the opposite direction (LengthFwd=-10), I get the face normal points to the outside.

jcad3

@martinRenou
Copy link
Member Author

The clipping indeed relies on the normals, so if they are not correct, it results in visual artifacts.

if I extrude in the opposite direction (LengthFwd=-10), I get the face normal points to the outside.

Interesting! Should we... reverse the normal depending on the sign of LengthFwd?

@martinRenou
Copy link
Member Author

martinRenou commented Jun 26, 2024

What's weird though is that it seems to be coming from the 3D refactoring pass? You were not only changing the code structure right? You also fixed the normals for STL files #353, could this impact the extrusion?

@trungleduc
Copy link
Member

What's weird though is that it seems to be coming from the 3D refactoring pass? You were not only changing the code structure right? You also fixed the normals for STL files #353, could this impact the extrusion?

I don't think it's related to the 3D pass, here is the result at 234f9bf, it works correctly

jcad5

Here is the result one commit after, at 85c0dde

jcad7

@trungleduc
Copy link
Member

Interesting! Should we... reverse the normal depending on the sign of LengthFwd?

Or just using double-sided material?

@trungleduc
Copy link
Member

It looks like the additional front and back meshes are made for the equivalent 3D solid tube since there are meshes for the top and bottom circles.

jcad33

@martinRenou
Copy link
Member Author

martinRenou commented Jun 26, 2024

I'll have a look. It seems the visibility toggle does not hide all the meshes created for a specific object (there are multiple ones to achieve the clipping).

I'm not sure we can use the double-sided material without breaking clipping. Using double-sided material would only workaround the wrong normals but not fix it, and wrong normals would result in broken clipping.

@trungleduc
Copy link
Member

trungleduc commented Jun 26, 2024

I'll have a look. It seems the visibility toggle does not hide all the meshes created for a specific object (there are multiple ones to achieve the clipping).

but we should not have the meshes at the top and bottom of the extruded shape ?

I'm not sure we can use the double-sided material without breaking clipping. Using double-sided material would only workaround the wrong normals but not fix it, and wrong normals would result in broken clipping.

For the extrusion case, I'm not sure what is the definition of correct normals, it's like when you push for one direction you will get the outside of the circle, but with the opposite direction you will get the inside

@martinRenou
Copy link
Member Author

but we should not have the meshes at the top and bottom of the extruded shape ?

I'm not sure what you mean by that?

I'm not sure what is the definition of correct normals

Always pointing outside?

Arf this is annoying... Clipping is what makes it difficult here because it needs proper normals, without this I would agree using double-sided materials and call it a day.

@martinRenou
Copy link
Member Author

As discussed offline, this issue comes from the clipping logic.

Clipping involves some logic to fill the gap left by the cut, this only makes sense for solid objects. However extrusion produces a non-solid object by default and that logic gets broken in that case, resulting in visual artifacts.

We should disable that logic for non-solid objects.

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

No branches or pull requests

2 participants