simplify the Graphics3d object of polyhedra

[fchapoton]

Currently, every face is in its own IndexFaceSet.

One can use one IndexFaceSet for all the faces together.

Also enhance the STL ouput by cutting no-triangular faces into triangles.

and make sure that all faces are oriented outwards.

This may also help simplify further the PLY export later.

CC: @ohanar @kcrisman @nilesjohnson @VivianePons @sagetrac-ams

Component: graphics

Keywords: days82

Author: Frédéric Chapoton

Branch/Commit: ed1815a

Reviewer: Alba Málaga

Issue created by migration from https://trac.sagemath.org/ticket/22144

[fchapoton]

New commits:

6c54142

simplify slightly the Graphics3d object of polyhedra

[fchapoton]

Branch: u/chapoton/22144

[fchapoton]

Commit: 6c54142

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

740b0a7

trac 22144 correct one doctest

[sagetrac-git]

Changed commit from 6c54142 to 740b0a7

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

443559c

simplify slightly the Graphics3d object of polyhedra

[sagetrac-git]

Changed commit from 740b0a7 to 443559c

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

976fd8e

simplify slightly the Graphics3d object of polyhedra

[sagetrac-git]

Changed commit from 443559c to 976fd8e

[sagetrac-git]

Changed commit from 976fd8e to 5f16820

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

443559c	simplify slightly the Graphics3d object of polyhedra
088e974	trac 22144 enhance stl ouput (for polyhedra in particular)
5f16820	Merge branch 'u/chapoton/22144' in alternative branch

[fchapoton]

comment:9

could you please try

sage: P = polytopes.truncated_octahedron()
sage: Q = P.plot().all[-1]
sage: Q.save('machin.stl')

it works for me. The second line isolate the IndexFaceSet from other bells and whistles (plot of edges and vertices)

useful: https://threejs.org/editor/

[sagetrac-ams]

comment:10

I tried your example.

For me, the second line still was a Graphics3DGroup so no face_list attribute that save(...stl) uses at this point in its code. So Q.save('machin.stl') fails with the error message 'Graphics3dGroup' object has no attribute 'face_list'.

However, P.plot().all[-1].all[i] will give a Graphics3D Object for every appropriate i ( here there are 14 faces, so i in range(14)). Then we can recover the full list of faces by putting all faces together.

sage: P = polytopes.truncated_octahedron()
sage: Q = P.plot().all[-1].all[:-1]
sage: my_face_list = [face for face_as_IFS in Q for face in face_as_IFS.face_list()] 

And then, I can call the save(....stl') method on an IndexFaceSet built out from this list.

sage: from sage.plot.plot3d.index_face_set import IndexFaceSet
sage: polytope_as_IFS = IndexFaceSet(my_face_list)
sage: polytope_as_IFS.save('machin.stl')

It will again fail, but for a different reason - the faces are not triangles.

[Edit] By the way, I was testing on the develop branch checked out from the github repository.

[fchapoton]

comment:11

well, of course (but I should have said that), I meant "try with the branch applied"

[sagetrac-ams]

comment:12

Replying to @fchapoton:

well, of course (but I should have said that), I meant "try with the branch applied"

Oh, my fault.

OK, after switching branches it did worked. Coool!!!

The text file looks fine and I verified that after import to Blender that there were not duplicated vertices and the normals are consistent.

[sagetrac-ams]

comment:13

Please, can you explain at what point the truncated octahedron faces are triangulated in this example?

It seems to me that you do it very early in the workflow, somewhere inside the plot call (anyway P has not method triangulate). But you don't need the triangulated faces for all 3D file formats exports. I agree you need triangular faces for stl, and 3ds, however triangular faces are not required for obj.

[sagetrac-ams]

Blender screenshot showing inconsistent normals on the exported stl.

[sagetrac-ams]

comment:14

Attachment: blender_wrong_normals.png

Sorry, it seems I overlooked some inconsistently oriented normals on the first test.

You can see after importing the machin.stl file generated with your example into Blender that part of the normals is pointing inwards and part is pointing outwards. Of course, it is possible to correct it afterwards, after exporting, but it would be nice to have normals of polyhedrons already consistent when saved to stl format.

[fchapoton]

comment:15

sorry, I was not available this morning, I should be there after lunch

yes, there is an issue about orientation of faces, that needs to be investigated.

[sagetrac-git]

Changed commit from 5f16820 to f71dba9

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

5715c6d	Merge branch 'u/chapoton/22144' in 7.5.rc3
f71dba9	trac 22144 trying to have normal vectors going outwards

[fchapoton]

comment:17

could you please try again with the new branch ?

[sagetrac-ams]

Blender screenshot showing that normals are not yet consistently pointing outwards.

[sagetrac-ams]

comment:18

Attachment: blender-still-wrong-normals.png

The export to stl still works but the normals are not yet consistent and pointing outwards.

[sagetrac-ams]

Blender screenshot of consistent normals pointing outwards.

[sagetrac-ams]

comment:24

Attachment: blender-consistent-normals.png

Ok, I see now that I was not looking at the good file. I looked at the old version of the stl, sorry.

The version generated with the new code has consistent normals pointing outwards.

congrats, so how did you got it ?

[fchapoton]

comment:25

ok, good.

I just reversed the list the vertices when needed. You can look at my latest commit by clicking on the link "commits" besides the name of the branch near the top of this page

[sagetrac-ams]

comment:27

Oh, thank you ! Actually, the link (commits) disappear when the site auto-updates with new inputs to the ticket's discussion (at least on this computer). Updating the site in the browser solves this issue.

[sagetrac-ams]

comment:28

How do you know that the facet_equation.A() is already pointing outwards ?

(cf. _init_solid_3d(self, polyhedron) in the commit f71dba9)

[fchapoton]

comment:29

Well, I assumed that the facet_equations are coming from the defining inequations of the polytope, so must be all coherent.

It was either everybody inwards or everybody outwards, so I tried < 0 and it works.

[sagetrac-ams]

comment:30

So you think it will be always everybody outwards ?

I was also thinking that this method will not translate easily to polyhedra that are not convex polytopes. But I am not even sure whether you can define those in Sage... and anyway, if it is possible to have, let say, a genus 1 polyhedron, it would be a different question and a different ticket.

[fchapoton]

comment:31

Well, I have not checked this, really, I admit. The idea is that inequalities are always stored as "vector . (?,?,?) + scalar >= 0". So taking the vector part of a facet should give the same normal in a coherent way, unless somebody somewhere has decided to change signs for some strange reason.

I have launched my patchbot to check that no doctest is broken anywhere else in sage.

Maybe you should check that some of the examples in Polyhedron? still plot as expected. In particular for open polyhedra, cones, etc. A priori, there should be no problem, but it's better to check a little bit.

[sagetrac-ams]

comment:32

Ok, I'll do that.

I have one more question, I cannot figure out in the code where do you actually triangulate the polyhedron's faces. I am worried about it because I am interested in both stl and obj formats for export and in obj I prefer to use quadris rather than tris.

[fchapoton]

comment:33

the chopping into triangles is done in the commit

trac 22144 enhance stl ouput (for polyhedra in particular)

and happens inside the stl string method, so will not affect other file format exports.

you can see the context by clicking on the commit and then enlarging the value of "context" in the little menu "diff options" on the right

[fchapoton]

comment:34

by the way, the better normals for facets should also have enhanced the obj output..

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d201dfb

trac 22144 adding a doctest for STL of many-sided faces

[sagetrac-git]

Changed commit from f71dba9 to d201dfb

[sagetrac-ams]

comment:36

I went trough all of the examples in plot3d? The plot looked reasonably good. But did you wanted to test the on-screen image or the stl export ?

None of those examples could export to stl before (I mean in Sage 7.4), but now, some of them manage to actually export :

sage: plot3d(lambda x, y: x^2 + y^2, (-2,2), (-2,2)).save('rho2.stl')

sage: plot3d(sin(x*y), (x, -pi, pi), (y, -pi, pi)).save('sinxy.stl')

and many others.

Those who do not work, have options added (other than plot_points or mesh or transformation), like for example:

sage: x,y = var('x,y')
sage: P = plot3d(x+y+sin(x*y), (x,-10,10),(y,-10,10), opacity=0.87, color='blue')
sage: Q = plot3d(x-2*y-cos(x*y),(x,-10,10),(y,-10,10),opacity=0.3,color='red')
sage: (P+Q).save('twoplanes.stl')

[fchapoton]

comment:37

No, I was just meaning to check the displayed plots. Thanks for doing that.

Your job as a reviewer, if you accept it:

• check that all doctests pass (wait for the bot, and run the tests yourself on the modified files)
• check that the doc is good and correctly formatted (should be ok, but you can think of improvements)
• be sure that this is an improvement over the existing situation
• ideally, be sure that there is no better solution to do the same thing (should not apply here)

This should be an easy review, I think. If you have more questions or suggestions, I am all ears. Once you are satisfied, set to positive review and put your name in the reviewer field at top of the page.

I also think one could keep further enhancements to later tickets.

[VivianePons]

Changed keywords from none to days82

[sagetrac-git]

Changed commit from d201dfb to ed1815a

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

ed1815a

trac 22144 fixing one doctest

[sagetrac-ams]

comment:40

There is one test

$ sage -t src/sage/geometry/polyhedron/plot.py

that fails.

Failed example:
    proj.polygons
Expected:
    [[2, 0, 1], [3, 0, 1], [3, 0, 2], [3, 1, 2]]
Got:
    [[1, 0, 2], [3, 0, 1], [2, 0, 3], [3, 1, 2]]

I looked through the example, and exported the stl in this precise example. It works, and normal are consistent, so maybe you should change the expected result accordingly in the doc.

[fchapoton]

comment:41

yes, done already

[sagetrac-ams]

Reviewer: Alba Málaga

[sagetrac-ams]

comment:42

Good job! All tests passed on my computer and the documentation looks good.

[fchapoton]

comment:43

a sequel in #22196, to use the much more efficient binary STL format instead

[vbraun]

Changed branch from u/chapoton/22144 to ed1815a