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

Helix difference extrusion yeilds staircase effect #489

Closed
kalle opened this issue Sep 20, 2019 · 31 comments · Fixed by #643
Closed

Helix difference extrusion yeilds staircase effect #489

kalle opened this issue Sep 20, 2019 · 31 comments · Fixed by #643

Comments

@kalle
Copy link

kalle commented Sep 20, 2019

System information

SolveSpace version: 3.0-7f9117b2bf0

Operating system: Fedora

Expected behavior

When selecting "force NURBS surfaces to triangle mesh" for a helix extrusion I though "chord tolerance" in the configuration menu would somehow influence the size of the triangles since it makes cylinders etc smoother.

Actual behavior

The slope of the helix in the attached model is very coarse, almost a staircase. In this model I have lowered the tolerance to 0.05% to no avail.

Additional information

If there is a way to control this, it should perhaps be more prominently positioned.

@whitequark
Copy link
Contributor

There is no attached model.

@kalle
Copy link
Author

kalle commented Sep 20, 2019

model.zip
That was fast! :-) Here it is! Sorry

@phkahler
Copy link
Member

My suspicion on this is the piece-wise edge down the middle of the helix is a good fit for the actual edge - they're both straight. That edge needs to be broken into more sections due to the twist of the surface, but twist is not taken into account by the chord tolerance setting.

If you use the icon at the top of the text window to display the mesh this issue is present even with regular NURBS rendering. It's probably less visible because the triangles are not flat-shaded in that mode.

@ruevs
Copy link
Member

ruevs commented May 8, 2020

With current master the NURBS booleans actually work better than the triangle mesh ones for this particular model. This is a first :-)

@ghost
Copy link

ghost commented May 8, 2020

I still see "stairs edges" on Helix surface:

  • chord tolerance: 0.01 %
    • pic.1
  • chord tolerance: 0.10 %
    • pic.2
  • chord tolerance: 0.001 %
    • pic.3

@phkahler
Copy link
Member

phkahler commented May 9, 2020

Those pictures are just rendering artifacts. Try showing the green triangle mesh via the icon on the text window toolbar. Everything is converted to triangles for display. The original problem looked like this:
auger6

Ideally the triangle mesh would have more detail down the central axis and not produce those large triangles that form stair-steps. If you export this one as STL for 3D printing, or OBJ for use in something else, those stair steps will actually be there, and I do consider that an issue.

When exporting as STEP file with NURBS surfaces I'm not sure what happens with the stair steps (not sure it's valid to helix around an edge), but the examples above with white edges will all be exported as smooth NURBS surfaces in spite of artifacts in SolveSpace rendering on screen.

I recently modified the code to force more triangles in the radial direction, but that just pushed the issue closer to the axis. Those edges need to be split into more pieces based on some surface criteria that doesn't exist. Edges are split independently of surfaces for now.

@ghost
Copy link

ghost commented May 9, 2020

Those pictures are just rendering artifacts.

Is there issue thread on this rendering issue?

@phkahler
Copy link
Member

phkahler commented May 9, 2020

Those pictures are just rendering artifacts.

Is there issue thread on this rendering issue?

It seems related to #379. These are not "solid edges" but they are edges of the triangular approximation of the surface. Seems like a tricky problem because those are supposed to show when one triangle is back facing and the other is front facing - that's probably how they show an "edge" around something like a torus even though there is no hard edge like on a cube. Twisted surfaces probably complicate that in ways that were never fully considered. It might also get better when the stair step issue here is resolved, as this one will require a better triangle mesh to fix.

@whitequark
Copy link
Contributor

Seems like a tricky problem because those are supposed to show when one triangle is back facing and the other is front facing - that's probably how they show an "edge" around something like a torus even though there is no hard edge like on a cube.

Yup, that's correct. In fact, you can see a similar issue with a torus, though it has far fewer artifacts:

Screenshot_20200509_193657

@phkahler
Copy link
Member

I assume @Evil-Spirit wrote the shader code. What lighting / surface shading model is used? We can turn off the white edge lines via toolbar button but the resulting rendering doesn't really show edges well.

@Evil-Spirit
Copy link
Collaborator

I recently ran into same [looking] issue. Solved by using the different formula of computing face normals.
NoteCAD - Microsoft Visual Studio 2020-05-10 10 37 44

But SolveSpace uses double precision and I have no idea if this is the same issue. Another reason - not correct triangulation. I mean, edges for the same vertices can be created in different way - so for one kind of surfaces edges can be good-looking, but for other cases you should filp edge.
3ds max - How to Flip the Edges in 3ds max_ - Graphic Design Stack Exchange - Google Chrome 2020-05-10 10 44 29

@ruevs
Copy link
Member

ruevs commented May 10, 2020

Another reason - not correct triangulation. I mean, edges for the same vertices can be created in different way - so for one kind of surfaces edges can be good-looking, but for other cases you should filp edge.
3ds max - How to Flip the Edges in 3ds max_ - Graphic Design Stack Exchange - Google Chrome 2020-05-10 10 44 29

This can help visually for curved surfaces, but so can shading (and it does in NURBS mode). The problem is that if the example model above is exported as STL and 3D printed/machined those big triangles "hanging from the center post" will be very obvious.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented May 10, 2020

No, this acually change geometry of surface because 4 vertices not lay in the same plane.

@phkahler
Copy link
Member

@ruevs actually your before image is considered "better" because the smallest angle in each triangle is larger ;-) But yeah, there are 2 ways to split a twisted quad. You can see in a torus there may be rings of quads split the other way. That's because the perimeter of each surface patch is triangulated with the generic method and not the quad-grid in my recent performance patch. Sometimes the perimeter does the diagonals the other direction.

@Evil-Spirit Theoretically only the last normalization is needed there. Also, I was wondering about the lighting model for the surfaces. It looks like there are 2 light sources (upper left, lower right) and just ambient (constant) + perfect diffuse lighting. Is that correct? There are some better lighting models that are only a little more complicated and would look good when the white edges are turned off.

@ghost
Copy link

ghost commented May 10, 2020

There are some better lighting models

JFTR, If in the future SolveSpace would get another lightning mode, could you keep "old lightning mode" (actual 2-sources lightning) as an option, please? Actual lightning is really cool if play with those preferences.

@ruevs
Copy link
Member

ruevs commented May 10, 2020

No, this acually change geometry of surface because 4 vertices not lay in the same plane.

I understand i changes the geometry, but in most cases (on "smooth" curved surfaces) it is mostly "cosmetic".

What I meant to say is that it can not help with this particular problem.

@Evil-Spirit
Copy link
Collaborator

@phkahler

Theoretically only the last normalization is needed there.

Practically you will get a problem when computing it in floating point mode for some kind of triangles. So, 3x normalization gives more precision (so hacky but works)
When I implemented lighting model, I've just provided the same way as it was in SolveSpace in FFP mode. Also, I made some things to avoid highlights caused by overlighting. Not actually know why.

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented May 10, 2020

@ruevs

I understand i changes the geometry, but in most cases (on "smooth" curved surfaces) it is mostly "cosmetic".

Not for 3d modelliing / 3d printing. If you are common with creating 3d models in e.g. 3d max, maya, blender, etc, you can know what turning edges can improve model quality. For the current case we can also improve quality of blueprints (export 2d to dxf/pdf etc). It is because silhouette can be changed by "turning edges".

@phkahler
Copy link
Member

There are some better lighting models

JFTR, If in the future SolveSpace would get another lightning mode, could you keep "old lightning mode" (actual 2-sources lightning) as an option, please? Actual lightning is really cool if play with those preferences.

I would think so. There is also Issue #524 asking for flat-color rendering. Designers can be very particular about changes to how parts are shown, I think taking away the current shading for something subjectively "better" would not be a good idea :-)

@Evil-Spirit
Copy link
Collaborator

Ah, there are two cases - bug from @phkahler and visual artifacts. I am talking about visual

@phkahler
Copy link
Member

phkahler commented Jul 1, 2020

I have an improvement for this. For helical extrusions it makes a minimum of 4 segments for all edges - even straight ones. I then force a minimum of 4x4 quads in grid triangulation for 2nd order and higher surfaces. For the above model at the new default CT settings it looks like this:
helix_grid

It's smart enough not to make the extra segments for revolve and lathe, but the grid triangulation is still making more triangles than needed in those cases - see the lower rim of the above where there are many narrow triangles where a simple pair could be. Geometrically that's correct and shouldn't hurt performance much but I'm still thinking about how to make it not happen. If this sounds like a good thing I'll make a PR after I let it bake a bit.

@whitequark
Copy link
Contributor

Sounds good to me!

@ruevs
Copy link
Member

ruevs commented Jul 1, 2020

The mesh on the cylinder at the bottom looks awful.
The cylindrical surface of the helix and the "pacman" face at the top are pretty bad as well.

@whitequark
Copy link
Contributor

It's still an improvement on the current state, right?

@ruevs
Copy link
Member

ruevs commented Jul 1, 2020

For the center axis of the helix - yes. The question is whether the degradation in other places is OK.
I'll play with it when I'm on a PC.

@phkahler
Copy link
Member

phkahler commented Jul 1, 2020

@whitequark the helix is better, but like @ruevs said... The mesh down there is actually fine geometrically, it's just more thin triangles than needed because the grid is used.

I did this by passing an optional flag down the MakePwlInto functions to indicate a curve is "twisted" which then invoked the "minimum 4 segments" thing I changed for circles. Now I'm thinking a better way would be to pass an optional "maximum dT" value that would force recursion until segments are less than that. That would be similar to how the minimum of 4 is done, but might allow helixes to specify shorter segments on the axis rather than just a flag to say "4".

Still experimenting. Thanks for the quick feedback.

phkahler added a commit to phkahler/solvespace that referenced this issue Jul 1, 2020
…4 segments and forcing grid triangulation to use a minimum of 4x4 quads. This helps with issue solvespace#489.
@phkahler
Copy link
Member

phkahler commented Jul 1, 2020

I'll play with it when I'm on a PC.

OK then I pushed it to my "better_helix" branch.

@phkahler
Copy link
Member

phkahler commented Jul 1, 2020

@ruevs without that change, just 4 segment edges NURBS:
nurbs_helix
Triangle mesh:
mesh_helix

Using the grid forces all the ugly stuff to the center strip where it also seems to behave better - making nicer fans to cover the differing segment counts. The grid also limits the length of the silhouette edges.

BTW I'm not entirely certain this creates a valid shell. I think the idea that helixing around a sketch line might be invalid came up in the original helix discussion. I managed to get it to do something kind of reasonable because it has to work for revolves. The edge on-axis does not create a surface in this case because it would be completely degenerate (zero area) so what is actually stitched together along the axis? I don't remember. It made my brain hurt to think about it.

@phkahler
Copy link
Member

phkahler commented Jul 2, 2020

Updated my branch to allow max_dt for pwl segments. This is cleaner code but doesn't really help. Some things I remembered / concluded:

  1. A helix created around a sketch line does not create a valid NURBS surface. The trim curves are not correct and will always result in naked edges (revolve is fine). The trims could be fixed but that's a decent rabbit hole. Force to triangle mesh seems to work OK for this case except the stair steps seen here.

  2. There will always be stair steps. The vertical line in the center is an edge of the surface, as such it must be broken into segments with a triangle formed from each segment. These triangles will always be vertical. The best we can ever do is create more/smaller of them. Grid triangulation helps because the center of the spiral is closer to vertical than the outer edge.

  3. Forcing a minimum grid size can make ugly triangles around the surface border. That might be fixable with some tweaks to ear triangulation to prefer better formed triangles.

  4. we probably want helical surfaces to be split into smaller than 90 degree NURBS patches. While experimenting I was able to see the nonlinearity shift as the number of segments changed. Unfortunately using smaller surfaces means more edges and more boolean issues, so that's probably a change for a later date.

No conclusion yet.

Edit: to clarify #1 about trim curves, they are geometrically correct but the connectivity - which surfaces share edges or even which span of an edge - varies depending on a lot of things and in general doesn't work as coded.

phkahler added a commit to phkahler/solvespace that referenced this issue Jul 7, 2020
…4 segments and forcing grid triangulation to use a minimum of 4x4 quads. This helps with issue solvespace#489. Also add a max_dt parameter for PWL creation and use that for helical edges.

comments and moved code
phkahler added a commit to phkahler/solvespace that referenced this issue Jul 7, 2020
…4 segments and forcing grid triangulation to use a minimum of 4x4 quads. This helps with issue solvespace#489. Also add a max_dt parameter for PWL creation and use that for helical edges.
phkahler added a commit to phkahler/solvespace that referenced this issue Jul 8, 2020
Force helix axis line to 8 segments. Forcing grid triangulation to use a minimum of 4 segment for degree>1. This helps with issue solvespace#489. Added a max_dt parameter for PWL creation and use that for helical edges.
@phkahler
Copy link
Member

phkahler commented Jul 8, 2020

I think I got it now. PR #643 will fix the stair steps without affecting the other surfaces. I've got the center axis broken into more pieces and dynamic grid subdivision based on the twist of the surface. The Auger6 model is shown here with a chord tolerance of 0.2:
Helix_fix

The radial edges on top (the Pac-Man) are rough but that's a different problem and does not happen with NURBS. The isues with NURBS edges along the axis remain but can be eliminated by drawing the Pac-Man and doing a helix with an axis perpendicular to the sketch plane.

The core issue here - the stair steps - has been much reduced.

phkahler added a commit to phkahler/solvespace that referenced this issue Jul 9, 2020
Resolve issue solvespace#489 helix has stairsteps.
Force helix axis line to 8 segments.
Grid triangulation to use a minimum of 4 segments for degree>1.
Adds twist dependence for grid triangulation with degree=1.
Added a max_dt parameter for PWL creation and use that for helical edges.
@phkahler
Copy link
Member

@Symbian9 and @Evil-Spirit The fix for this issue is also going to eliminate most of the false silhouette edges on these surfaces. It should be merged soon and will close this issue.

@phkahler phkahler linked a pull request Jul 11, 2020 that will close this issue
phkahler added a commit that referenced this issue Jul 11, 2020
Resolve issue #489 helix has stairsteps.
Force helix axis line to 8 segments.
Grid triangulation to use a minimum of 4 segments for degree>1.
Adds twist dependence for grid triangulation with degree=1.
Added a max_dt parameter for PWL creation and use that for helical edges.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants