Opened 2 years ago
Closed 23 months ago
#22144 closed enhancement (fixed)
simplify the Graphics3d object of polyhedra
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage7.5 
Component:  graphics  Keywords:  days82 
Cc:  ohanar, kcrisman, niles, VivianePons, ams  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Alba Málaga 
Report Upstream:  N/A  Work issues:  
Branch:  ed1815a (Commits)  Commit:  ed1815abd41de4c68c04a3224647099f9e02d8fe 
Dependencies:  Stopgaps: 
Description (last modified by )
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 notriangular faces into triangles.
and make sure that all faces are oriented outwards.
This may also help simplify further the PLY export later.
Attachments (3)
Change History (47)
comment:1 Changed 2 years ago by
 Branch set to u/chapoton/22144
 Commit set to 6c54142cffa2e7fd61bff6edac32b3ee277bb44d
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 Commit changed from 6c54142cffa2e7fd61bff6edac32b3ee277bb44d to 740b0a75791503690f4236530847dc2cb10dad98
Branch pushed to git repo; I updated commit sha1. New commits:
740b0a7  trac 22144 correct one doctest

comment:3 Changed 2 years ago by
 Commit changed from 740b0a75791503690f4236530847dc2cb10dad98 to 443559cfc01c639ba280bb132f5f32606a90a2d4
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
443559c  simplify slightly the Graphics3d object of polyhedra

comment:4 Changed 2 years ago by
 Commit changed from 443559cfc01c639ba280bb132f5f32606a90a2d4 to 976fd8eaa5bd9815f71e0cfaf18811a8c6fce1dc
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
976fd8e  simplify slightly the Graphics3d object of polyhedra

comment:5 Changed 2 years ago by
 Commit changed from 976fd8eaa5bd9815f71e0cfaf18811a8c6fce1dc to 5f16820fa2dfb05e78674028bd097ec4ca0d55ec
comment:6 Changed 2 years ago by
 Cc ohanar kcrisman niles added
 Description modified (diff)
comment:7 Changed 2 years ago by
 Cc VivianePons added
comment:8 Changed 2 years ago by
 Cc ams added
comment:9 Changed 2 years ago by
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/
comment:10 Changed 2 years ago by
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.
comment:11 followup: ↓ 12 Changed 2 years ago by
well, of course (but I should have said that), I meant "try with the branch applied"
comment:12 in reply to: ↑ 11 Changed 2 years ago by
Replying to chapoton:
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.
comment:13 Changed 2 years ago by
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
.
comment:14 Changed 2 years ago by
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.
comment:15 Changed 2 years ago by
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.
comment:16 Changed 2 years ago by
 Commit changed from 5f16820fa2dfb05e78674028bd097ec4ca0d55ec to f71dba9c48b8dc477d00a6a76397fb3a95bc7619
comment:17 Changed 2 years ago by
could you please try again with the new branch ?
Changed 2 years ago by
Blender screenshot showing that normals are not yet consistently pointing outwards.
comment:18 Changed 2 years ago by
comment:19 Changed 2 years ago by
really ? ...surprising. Did you rebuild sage ? using "sage br"
When I try this using https://threejs.org/editor/ it seems that all facets are correctly oriented..
comment:20 Changed 2 years ago by
Yes, I did rebuild. I will try uploading my model into the website you listed.
Can you explain how do you test using https://threejs.org/editor/ ? Where do you upload the stl, or do you use directly the faces as lists of vertices ?
comment:21 Changed 2 years ago by
Wait, wait, what does it mean to rebuild using sage br
? The way I did the rebuild was
$ git pull trac u/chapoton/22144 $ make
Should I pass a parameter to make
, like forcesomething
or sagebr
in order to ensure that it recompiles the interesting function ?
comment:22 Changed 2 years ago by
I upload the stl (using the file menu / import). When facets were wrong, some of them did no appear until one ask (in the geometry panel) to show every triangle with both sides.
comment:23 Changed 2 years ago by
"sage br" should be equivalent to "make" here. It does not care about building the doc, which is faster in general. In some sense "sage br" is like a smallscale "make", that is sufficient very often when you do not switch branch
comment:24 Changed 2 years ago by
comment:25 Changed 2 years ago by
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
comment:26 Changed 2 years ago by
 Description modified (diff)
comment:27 Changed 2 years ago by
Oh, thank you ! Actually, the link (commits) disappear when the site autoupdates with new inputs to the ticket's discussion (at least on this computer). Updating the site in the browser solves this issue.
comment:28 Changed 2 years ago by
How do you know that the facet_equation.A()
is already pointing outwards ?
(cf. _init_solid_3d(self, polyhedron)
in the commit f71dba9c48b8dc477d00a6a76397fb3a95bc7619)
comment:29 Changed 2 years ago by
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.
comment:30 Changed 2 years ago by
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.
comment:31 Changed 2 years ago by
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.
comment:32 Changed 2 years ago by
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.
comment:33 Changed 2 years ago by
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
comment:34 Changed 2 years ago by
by the way, the better normals for facets should also have enhanced the obj output..
comment:35 Changed 2 years ago by
 Commit changed from f71dba9c48b8dc477d00a6a76397fb3a95bc7619 to d201dfbbf5f4dc6d95f21b978ddcc7744f70ac56
Branch pushed to git repo; I updated commit sha1. New commits:
d201dfb  trac 22144 adding a doctest for STL of manysided faces

comment:36 Changed 2 years ago by
I went trough all of the examples in plot3d?
The plot looked reasonably good. But did you wanted to test the onscreen 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(x2*ycos(x*y),(x,10,10),(y,10,10),opacity=0.3,color='red') sage: (P+Q).save('twoplanes.stl')
comment:37 Changed 2 years ago by
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.
comment:38 Changed 2 years ago by
 Keywords days82 added
comment:39 Changed 2 years ago by
 Commit changed from d201dfbbf5f4dc6d95f21b978ddcc7744f70ac56 to ed1815abd41de4c68c04a3224647099f9e02d8fe
Branch pushed to git repo; I updated commit sha1. New commits:
ed1815a  trac 22144 fixing one doctest

comment:40 Changed 2 years ago by
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.
comment:41 Changed 2 years ago by
yes, done already
comment:42 Changed 2 years ago by
 Reviewers set to Alba Málaga
 Status changed from needs_review to positive_review
Good job! All tests passed on my computer and the documentation looks good.
comment:43 Changed 2 years ago by
a sequel in #22196, to use the much more efficient binary STL format instead
comment:44 Changed 23 months ago by
 Branch changed from u/chapoton/22144 to ed1815abd41de4c68c04a3224647099f9e02d8fe
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
simplify slightly the Graphics3d object of polyhedra