Comment most of SymbolicObj3 back in. #67

Open
wants to merge 1 commit into
from

Projects

None yet

6 participants

@jbj
jbj commented Oct 3, 2012

3D rendering seems to have been buggy since April, between versions 0.0.1 and 0.0.2. Objects with sharp edges, such as cubes and cylinders, get rendered with those edges "crinkled". You can see this by rendering the sample code of the first 3D object in README.md with the latest version of ImplicitCAD. It looks much uglier than the picture in the readme.

I bisected this to have happened in commit 14af939, where most of the SymbolicObj3.hs was commented out. This seems to have disabled symbolic object rendering, so everything is done with marching cubes.

I don't know if this is the right fix, but at least this makes the examples from the readme pretty again. I haven't tested whether all the code that gets revived here actually works, but at least it compiles without problems.

Jonas B. Jensen Comment most of SymbolicObj3 back in.
This big lump of code seems to have been commented out as a temporary measure
long ago and never reinstated. Its absence made all objects have crinkled
corners.

This reverts commit 14af939.
98f5455
@bgamari

Has anything happened with this? Is it clear to anyone whether this is a sane solution?

@bgamari

@colah, any opinion on this? It looks like 14af939 was originally intended as an optimization. It seems strange that the author merely commented the code out and not deleted it outright if this is intended to be a permanent optimization.

@colah
Owner

Ack, I completely dropped the ball on this. Sorry, @jbj, @bgamari!

The explanation of this code is kind of complicated. Originally, it fixed some simple cases where ImplicitCAD's rendering system was flawed. Eventually I rewrote the rendering system to try and make this unnecessary. This code made it substantially trickier to actually debug the new rendering system, because one had to be careful about whether it was hitting one of the symbolic rendering cases instead of the main rendering system... Since it wasn't clear whether we would want this in the long term, I commented it out.

It still really isn't clear, from my perspective. The benefits are a lot smaller and I still have ideas for making the rendering system better. On the other hand, the only real cost to this code is that it needs to be commented out for some types of development...

Some of the code needs to get rewritten, if we want to use it again, since it depends on some historical rendering stuff...

@bgamari

For the sake of cleanliness, perhaps we should just kill the code for now? It is still available in the revision history for future reference and clearly isn't usable as is at the moment.

@jbj

The purpose of this pull request was to fix a serious regression that has decreased rendering quality and increased polygon count a lot. It's probably not the right long-term fix, and I probably should have filed an issue about the regression separately, but the problem is still there.

Compare the following two images. They are both renderings of the same source code -- the very first 3D example in README.md. The first is rendered with symbolic rendering enabled (by this pull request), and it looks the same as the one you show in the readme. The second is rendered with the latest version of ImplicitCAD.

linextr-sym

linextr-cubes

If you don't want to enable symbolic rendering again, I think you should at least update the pictures in the readme to reflect how current versions of ImplicitCAD render that code.

@colah
Owner

That's a good point, though in fairness, a higher rendering quality would go a long ways to fix it. (And I prefer our new ray traced pictures, anyways :P )

What I really need to do is find a way to set build flags that enable and disable certain pieces of code -- eg. this is a pain for development, but nice to enable for releases. I'll research it and get back to you on this on Saturday.

@bgamari

@colah, there is always -XCPP and a Cabal flag. Probably not a great solution in most instances, but for cases like this where the solution is temporary anyways it could suffice.

@colah
Owner

Yeah, -XCPP may be the best solution... Other stuff occupied my time today and so I haven't really had time to research this, but at a minimum, I will temporarily re-enable symbolic rendering for the next release.

@jbj, I'd like to apologize for my poor handling of your pull request. I don't really have any experience running an open source project, and this is one of a number of mistakes I've made. I will be spending some time thinking about mistakes I've made and how I can improve (and am open to comments from anyone, email me).

@jbj

@colah, don't worry about it. I'm all too familiar with the guilty conscience that paradoxically goes with giving away your work for free.

@ksjogo

Deleting these lines by hand is erroring for me.

Graphics/Implicit/Export/SymbolicObj3.hs:41:29: Not in scope:
S.+'

Graphics/Implicit/Export/SymbolicObj3.hs:41:38: Not in scope: `S.+'

Graphics/Implicit/Export/SymbolicObj3.hs:41:47: Not in scope: `S.+'

Graphics/Implicit/Export/SymbolicObj3.hs:49:44:
Not in scope: `S.⋯*'

Graphics/Implicit/Export/SymbolicObj3.hs:49:54:
Not in scope: `S.⋯*'

Graphics/Implicit/Export/SymbolicObj3.hs:49:64:
Not in scope: S.⋯*'
Is there another way or where can I enable the ray tracer?

@silky

any updated thoughts on this?

to answer @ksjogo ; the SaneOperators class has been removed and they've moved to Data.VectorSapce where the "S.+" operator is now the ^+^ operator from that library

see this commit - b5aaaed

@mmachenry
Collaborator

This has compile-time errors.

Graphics/Implicit/Export/SymbolicObj3.hs:35:25: Not in scope: ‘S.+’

See @silky's comment above. I can't just merge it like this. Also it's trying to merge against a rather old version of the file that used to have tabs so it has a huge amount of conflicts. Basically every line in the file.

@silky

before i commented i actually got (essentially) this working, but it didn't result in any noticable difference

i also note that in general STL generation from ImplicitCAD is very slow. for the logo from - https://github.com/silky/haskmas - it takes about 2 minutes for ImplicitCAD to generate the stl, whereas outputing scad code for, say, OpenSCAD to process and letting that general the STL (which takes 5 seconds or so) is much more efficient

and furthermore, even the STL that ImplicitCAD generates isn't particularly "nice"; it has several artefacts (i.e. big zero-height sheets between segments, etc).

so i think something a bit more substantial is required ... (that said, ImplicitCAD -> .scad is a nice process.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment