discuss@lists.openscad.org

OpenSCAD general discussion Mailing-list

View all threads

I'm still confused about Manifold

SL
Steve Lelievre
Sat, Mar 23, 2024 4:46 PM

Yay!

On 2024-03-22 8:48 p.m., Marius Kintel wrote:

Yay! On 2024-03-22 8:48 p.m., Marius Kintel wrote: > Fixed in https://github.com/openscad/openscad/pull/5062 >
RW
Rogier Wolff
Mon, Mar 25, 2024 10:18 AM

On Sat, Mar 23, 2024 at 09:46:08AM -0700, Steve Lelievre via Discuss wrote:

Yay!

On 2024-03-22 8:48 p.m., Marius Kintel wrote:

Would it be an idea to fix this in a different way?

Just check for identical vertices at both ends and then issue the
reduced-triangle-count stuff? This would mean that say a highdef
cylinder with a very small top so that points start getting
snapped-to-the-same-gridpoint would also get handled correctly.

(Just thinking out loud if maybe a sligthly different way would
generalize better and fix a few more issues that are rare anyway (*)).

Roger.

(*) Might take years before someone runs into it, but defensive
programming would allow us to fix the issue before it happens.

--
** R.E.Wolff@BitWizard.nl ** https://www.BitWizard.nl/ ** +31-15-2049110 **
**    Delftechpark 11 2628 XJ  Delft, The Netherlands.  KVK: 27239233    **
f equals m times a. When your f is steady, and your m is going down
your a is going up.  -- Chris Hadfield about flying up the space shuttle.

On Sat, Mar 23, 2024 at 09:46:08AM -0700, Steve Lelievre via Discuss wrote: > Yay! > > On 2024-03-22 8:48 p.m., Marius Kintel wrote: > > Fixed in https://github.com/openscad/openscad/pull/5062 Would it be an idea to fix this in a different way? Just check for identical vertices at both ends and then issue the reduced-triangle-count stuff? This would mean that say a highdef cylinder with a very small top so that points start getting snapped-to-the-same-gridpoint would also get handled correctly. (Just thinking out loud if maybe a sligthly different way would generalize better and fix a few more issues that are rare anyway (*)). Roger. (*) Might take years before someone runs into it, but defensive programming would allow us to fix the issue before it happens. -- ** R.E.Wolff@BitWizard.nl ** https://www.BitWizard.nl/ ** +31-15-2049110 ** ** Delftechpark 11 2628 XJ Delft, The Netherlands. KVK: 27239233 ** f equals m times a. When your f is steady, and your m is going down your a is going up. -- Chris Hadfield about flying up the space shuttle.
MK
Marius Kintel
Mon, Mar 25, 2024 2:21 PM

On Mar 25, 2024, at 06:18, Rogier Wolff R.E.Wolff@BitWizard.nl wrote:

Would it be an idea to fix this in a different way?

Just check for identical vertices at both ends and then issue the
reduced-triangle-count stuff? This would mean that say a highdef
cylinder with a very small top so that points start getting
snapped-to-the-same-gridpoint would also get handled correctly.

(Just thinking out loud if maybe a sligthly different way would
generalize better and fix a few more issues that are rare anyway (*)).

That’s a different problem: The fix above fixed a topology issue with a built-in shape, where the same vertex was used multiple times in a polygon causing degenerate topology. If the upper radius gets sufficiently small, it will still generate a valid topology, and it’s up to the geometry engine to sort out how to merge the vertices. We don’t pass this through any quantization at this stage.

But: If you find an actual problem, please open a GitHub issue - we’re working through all the remaining quirks these days, to be able to deem Manifold a stable backend. For a list of open issues, see https://github.com/openscad/openscad/issues/4825

In fact, if you find any geometry issue which used to be broken to still be broken with Manifold, that’s also worth highlighting as a new GitHub issue (or ping me on an old issue).

-Marius

> On Mar 25, 2024, at 06:18, Rogier Wolff <R.E.Wolff@BitWizard.nl> wrote: >> >>> Fixed in https://github.com/openscad/openscad/pull/5062 > > Would it be an idea to fix this in a different way? > > Just check for identical vertices at both ends and then issue the > reduced-triangle-count stuff? This would mean that say a highdef > cylinder with a very small top so that points start getting > snapped-to-the-same-gridpoint would also get handled correctly. > > (Just thinking out loud if maybe a sligthly different way would > generalize better and fix a few more issues that are rare anyway (*)). > That’s a different problem: The fix above fixed a topology issue with a built-in shape, where the same vertex was used multiple times in a polygon causing degenerate topology. If the upper radius gets sufficiently small, it will still generate a valid topology, and it’s up to the geometry engine to sort out how to merge the vertices. We don’t pass this through any quantization at this stage. But: If you find an actual problem, please open a GitHub issue - we’re working through all the remaining quirks these days, to be able to deem Manifold a stable backend. For a list of open issues, see https://github.com/openscad/openscad/issues/4825 In fact, if you find _any_ geometry issue which used to be broken to still be broken with Manifold, that’s also worth highlighting as a new GitHub issue (or ping me on an old issue). -Marius
RW
Rogier Wolff
Mon, Mar 25, 2024 4:10 PM

On Mon, Mar 25, 2024 at 10:21:48AM -0400, Marius Kintel via Discuss wrote:

On Mar 25, 2024, at 06:18, Rogier Wolff R.E.Wolff@BitWizard.nl wrote:

Would it be an idea to fix this in a different way?

Just check for identical vertices at both ends and then issue the
reduced-triangle-count stuff? This would mean that say a highdef
cylinder with a very small top so that points start getting
snapped-to-the-same-gridpoint would also get handled correctly.

(Just thinking out loud if maybe a sligthly different way would
generalize better and fix a few more issues that are rare anyway (*)).

That’s a different problem: The fix above fixed a topology issue
with a built-in shape, where the same vertex was used multiple times
in a polygon causing degenerate topology. If the upper radius gets
sufficiently small, it will still generate a valid topology, and
it’s up to the geometry engine to sort out how to merge the
vertices. We don’t pass this through any quantization at this
stage.

Your patch made the code longer -> more effort to maintain in the long
run. So I was thinking if I could find a shorter formulation and it is
possible that "my way" of fixing that code a) fixes it (just like your
patch does) and b) is shorter. Apparently my argument c) might fix a
problem that hasn't occurred yet, isn't valid.

Roger. 

--
** R.E.Wolff@BitWizard.nl ** https://www.BitWizard.nl/ ** +31-15-2049110 **
**    Delftechpark 11 2628 XJ  Delft, The Netherlands.  KVK: 27239233    **
f equals m times a. When your f is steady, and your m is going down
your a is going up.  -- Chris Hadfield about flying up the space shuttle.

On Mon, Mar 25, 2024 at 10:21:48AM -0400, Marius Kintel via Discuss wrote: > > On Mar 25, 2024, at 06:18, Rogier Wolff <R.E.Wolff@BitWizard.nl> wrote: > >> > >>> Fixed in https://github.com/openscad/openscad/pull/5062 > > > > Would it be an idea to fix this in a different way? > > > > Just check for identical vertices at both ends and then issue the > > reduced-triangle-count stuff? This would mean that say a highdef > > cylinder with a very small top so that points start getting > > snapped-to-the-same-gridpoint would also get handled correctly. > > > > (Just thinking out loud if maybe a sligthly different way would > > generalize better and fix a few more issues that are rare anyway (*)). > > > That’s a different problem: The fix above fixed a topology issue > with a built-in shape, where the same vertex was used multiple times > in a polygon causing degenerate topology. If the upper radius gets > sufficiently small, it will still generate a valid topology, and > it’s up to the geometry engine to sort out how to merge the > vertices. We don’t pass this through any quantization at this > stage. Your patch made the code longer -> more effort to maintain in the long run. So I was thinking if I could find a shorter formulation and it is possible that "my way" of fixing that code a) fixes it (just like your patch does) and b) is shorter. Apparently my argument c) might fix a problem that hasn't occurred yet, isn't valid. Roger. -- ** R.E.Wolff@BitWizard.nl ** https://www.BitWizard.nl/ ** +31-15-2049110 ** ** Delftechpark 11 2628 XJ Delft, The Netherlands. KVK: 27239233 ** f equals m times a. When your f is steady, and your m is going down your a is going up. -- Chris Hadfield about flying up the space shuttle.
MK
Marius Kintel
Mon, Mar 25, 2024 4:47 PM

On Mar 25, 2024, at 12:10, Rogier Wolff R.E.Wolff@BitWizard.nl wrote:

Your patch made the code longer -> more effort to maintain in the long
run. So I was thinking if I could find a shorter formulation and it is
possible that "my way" of fixing that code a) fixes it (just like your
patch does) and b) is shorter.

Yes, there’s a fine balance between short and long in terms of future maintenance efforts. If you have time to rewrite it, we’re accepting pull requests :)

Apparently my argument c) might fix a problem that hasn't occurred yet, isn't valid.

It’s a valid argument in general, but I think it’s mostly valuable for user-provided polyhedrons. If you read the code you’ll see where the grid point quantization kicks in. I believe this is already solved for that use-case.
I think if a problem hasn’t occurred yet, but you suspect it’s a real problem, it should be possible to write a test case exhibiting the issue. Until then it’s too hard to anticipate and fix untested problems.

-Marius

> On Mar 25, 2024, at 12:10, Rogier Wolff <R.E.Wolff@BitWizard.nl> wrote: > > Your patch made the code longer -> more effort to maintain in the long > run. So I was thinking if I could find a shorter formulation and it is > possible that "my way" of fixing that code a) fixes it (just like your > patch does) and b) is shorter. Yes, there’s a fine balance between short and long in terms of future maintenance efforts. If you have time to rewrite it, we’re accepting pull requests :) > Apparently my argument c) might fix a problem that hasn't occurred yet, isn't valid. > It’s a valid argument in general, but I think it’s mostly valuable for user-provided polyhedrons. If you read the code you’ll see where the grid point quantization kicks in. I believe this is already solved for that use-case. I think if a problem hasn’t occurred yet, but you suspect it’s a real problem, it should be possible to write a test case exhibiting the issue. Until then it’s too hard to anticipate and fix untested problems. -Marius
RW
Rogier Wolff
Wed, Mar 27, 2024 2:50 PM

On Mon, Mar 25, 2024 at 12:47:10PM -0400, Marius Kintel via Discuss wrote:

On Mar 25, 2024, at 12:10, Rogier Wolff R.E.Wolff@BitWizard.nl wrote:

Your patch made the code longer -> more effort to maintain in the long
run. So I was thinking if I could find a shorter formulation and it is
possible that "my way" of fixing that code a) fixes it (just like your
patch does) and b) is shorter.

Yes, there’s a fine balance between short and long in terms of
future maintenance efforts. If you have time to rewrite it, we’re
accepting pull requests :)

80%+ of the time needed to do such things is "getting to know the
code" around the part you need to modify.

e.g. The "kernel malloc" in Linux was 3 hours of "getting to know the
code" and then about half an hour of coding.

It was mostly a suggestion that if you'd say: "Oh, hadn't thought
about it, yep that'd be shorter, I can rewrite that in 5 minutes
flat!"

Such "thinking out loud" things might lead to nothing in say 90% of
cases, but be incredibly useful in the remaining 10%, so I think we
should not discourage it too much.

Roger. 

--
** R.E.Wolff@BitWizard.nl ** https://www.BitWizard.nl/ ** +31-15-2049110 **
**    Delftechpark 11 2628 XJ  Delft, The Netherlands.  KVK: 27239233    **
f equals m times a. When your f is steady, and your m is going down
your a is going up.  -- Chris Hadfield about flying up the space shuttle.

On Mon, Mar 25, 2024 at 12:47:10PM -0400, Marius Kintel via Discuss wrote: > > On Mar 25, 2024, at 12:10, Rogier Wolff <R.E.Wolff@BitWizard.nl> wrote: > > > > Your patch made the code longer -> more effort to maintain in the long > > run. So I was thinking if I could find a shorter formulation and it is > > possible that "my way" of fixing that code a) fixes it (just like your > > patch does) and b) is shorter. > Yes, there’s a fine balance between short and long in terms of > future maintenance efforts. If you have time to rewrite it, we’re > accepting pull requests :) 80%+ of the time needed to do such things is "getting to know the code" around the part you need to modify. e.g. The "kernel malloc" in Linux was 3 hours of "getting to know the code" and then about half an hour of coding. It was mostly a suggestion that if you'd say: "Oh, hadn't thought about it, yep that'd be shorter, I can rewrite that in 5 minutes flat!" Such "thinking out loud" things might lead to nothing in say 90% of cases, but be incredibly useful in the remaining 10%, so I think we should not discourage it too much. Roger. -- ** R.E.Wolff@BitWizard.nl ** https://www.BitWizard.nl/ ** +31-15-2049110 ** ** Delftechpark 11 2628 XJ Delft, The Netherlands. KVK: 27239233 ** f equals m times a. When your f is steady, and your m is going down your a is going up. -- Chris Hadfield about flying up the space shuttle.