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: sage-7.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 chapoton)

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.

Attachments (3)

blender_wrong_normals.png (33.2 KB) - added by ams 2 years ago.
Blender screenshot showing inconsistent normals on the exported stl.
blender-still-wrong-normals.png (6.2 KB) - added by ams 2 years ago.
Blender screenshot showing that normals are not yet consistently pointing outwards.
blender-consistent-normals.png (7.0 KB) - added by ams 2 years ago.
Blender screenshot of consistent normals pointing outwards.

Download all attachments as: .zip

Change History (47)

comment:1 Changed 2 years ago by chapoton

  • Branch set to u/chapoton/22144
  • Commit set to 6c54142cffa2e7fd61bff6edac32b3ee277bb44d
  • Status changed from new to needs_review

New commits:

6c54142simplify slightly the Graphics3d object of polyhedra

comment:2 Changed 2 years ago by git

  • Commit changed from 6c54142cffa2e7fd61bff6edac32b3ee277bb44d to 740b0a75791503690f4236530847dc2cb10dad98

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

740b0a7trac 22144 correct one doctest

comment:3 Changed 2 years ago by git

  • Commit changed from 740b0a75791503690f4236530847dc2cb10dad98 to 443559cfc01c639ba280bb132f5f32606a90a2d4

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

443559csimplify slightly the Graphics3d object of polyhedra

comment:4 Changed 2 years ago by git

  • Commit changed from 443559cfc01c639ba280bb132f5f32606a90a2d4 to 976fd8eaa5bd9815f71e0cfaf18811a8c6fce1dc

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

976fd8esimplify slightly the Graphics3d object of polyhedra

comment:5 Changed 2 years ago by git

  • Commit changed from 976fd8eaa5bd9815f71e0cfaf18811a8c6fce1dc to 5f16820fa2dfb05e78674028bd097ec4ca0d55ec

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

443559csimplify slightly the Graphics3d object of polyhedra
088e974trac 22144 enhance stl ouput (for polyhedra in particular)
5f16820Merge branch 'u/chapoton/22144' in alternative branch

comment:6 Changed 2 years ago by chapoton

  • Cc ohanar kcrisman niles added
  • Description modified (diff)

comment:7 Changed 2 years ago by chapoton

  • Cc VivianePons added

comment:8 Changed 2 years ago by VivianePons

  • Cc ams added

comment:9 Changed 2 years ago by chapoton

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/

Last edited 2 years ago by chapoton (previous) (diff)

comment:10 Changed 2 years ago by ams

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.

Last edited 2 years ago by ams (previous) (diff)

comment:11 follow-up: Changed 2 years ago by chapoton

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 ams

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 ams

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.

Changed 2 years ago by ams

Blender screenshot showing inconsistent normals on the exported stl.

comment:14 Changed 2 years ago by ams

Sorry, it seems I overlooked some inconsistently oriented normals on the first test.      Blender screenshot showing inconsistent normals on the exported stl.

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 chapoton

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 git

  • Commit changed from 5f16820fa2dfb05e78674028bd097ec4ca0d55ec to f71dba9c48b8dc477d00a6a76397fb3a95bc7619

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

5715c6dMerge branch 'u/chapoton/22144' in 7.5.rc3
f71dba9trac 22144 trying to have normal vectors going outwards

comment:17 Changed 2 years ago by chapoton

could you please try again with the new branch ?

Changed 2 years ago by ams

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

comment:18 Changed 2 years ago by ams

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

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

comment:19 Changed 2 years ago by chapoton

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..

Last edited 2 years ago by chapoton (previous) (diff)

comment:20 Changed 2 years ago by ams

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 ?

Last edited 2 years ago by ams (previous) (diff)

comment:21 Changed 2 years ago by ams

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 --force-something or --sage-br in order to ensure that it recompiles the interesting function ?

comment:22 Changed 2 years ago by chapoton

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.

Last edited 2 years ago by chapoton (previous) (diff)

comment:23 Changed 2 years ago by chapoton

"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 small-scale "make", that is sufficient very often when you do not switch branch

Changed 2 years ago by ams

Blender screenshot of consistent normals pointing outwards.

comment:24 Changed 2 years ago by ams

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.

Blender screenshot of consistent normals pointing outwards.

congrats, so how did you got it ?

comment:25 Changed 2 years ago by chapoton

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 chapoton

  • Description modified (diff)

comment:27 Changed 2 years ago by ams

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.

comment:28 Changed 2 years ago by ams

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

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

Last edited 2 years ago by ams (previous) (diff)

comment:29 Changed 2 years ago by chapoton

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.

Last edited 2 years ago by chapoton (previous) (diff)

comment:30 Changed 2 years ago by ams

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 chapoton

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 ams

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 chapoton

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

Last edited 2 years ago by chapoton (previous) (diff)

comment:34 Changed 2 years ago by chapoton

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

comment:35 Changed 2 years ago by git

  • Commit changed from f71dba9c48b8dc477d00a6a76397fb3a95bc7619 to d201dfbbf5f4dc6d95f21b978ddcc7744f70ac56

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

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

comment:36 Changed 2 years ago by ams

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')

comment:37 Changed 2 years ago by chapoton

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 VivianePons

  • Keywords days82 added

comment:39 Changed 2 years ago by git

  • Commit changed from d201dfbbf5f4dc6d95f21b978ddcc7744f70ac56 to ed1815abd41de4c68c04a3224647099f9e02d8fe

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

ed1815atrac 22144 fixing one doctest

comment:40 Changed 2 years ago by ams

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 chapoton

yes, done already

comment:42 Changed 2 years ago by ams

  • 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 chapoton

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

comment:44 Changed 23 months ago by vbraun

  • Branch changed from u/chapoton/22144 to ed1815abd41de4c68c04a3224647099f9e02d8fe
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.