Refinement of various scene functions #1385
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1385 +/- ##
==========================================
+ Coverage 60.21% 60.24% +0.02%
==========================================
Files 207 207
Lines 22248 22244 -4
==========================================
+ Hits 13397 13401 +4
+ Misses 8851 8843 -8 ☔ View full report in Codecov by Sentry. |
|
the commented out part is (mostly) related to labels, which i think we should handle independently from the individual types of objects in order to avoid repetition. have left it there as a reminder... |
|
haven't checked GH, but the default behaviour of all functionality remains the same. so i don't expect any problems... |
in any case, the removed code paths were not (obviously) accessible nor documented so i assume there is no real issue. |
There was a problem hiding this comment.
I added some inline comments, but the biggest prob right now is that GH mesh object doesn't work at all (I guess it needs the same refactoring regarding vertex_xyz).
The other issue is that the removal of draw_vertexlabels, draw_edgelabels, draw_facelabels, draw_vertexnormals, draw_facenormals is not so undocumented as mentioned. We use them pretty much every single time in workshops and intro classes to compas. I don't think it can just go away.
src/compas/scene/scene.py
Outdated
| def clear_context(self, guids=None): | ||
| # type: (list | None) -> None | ||
| """Clear all objects from the visualisation context.""" | ||
| clear(guids) | ||
|
|
||
| clear() | ||
| def clear(self, clear_scene=True, clear_context=True): | ||
| # type: (bool, bool) -> None | ||
| """Clear all objects inside the scene. | ||
|
|
||
| def clear_objects(self): | ||
| # type: () -> None | ||
| """Clear all objects inside the scene.""" | ||
| Parameters | ||
| ---------- | ||
| context : bool, optional | ||
| Clear all objects from the context as well. |
There was a problem hiding this comment.
This could indeed have a simple test to cover it, because the clear/clear_context is already not clear enough by the naming, so, at least adding a test would ensure that this is not broken in the future by accident.
| @@ -215,7 +151,7 @@ def draw(self): | |||
| disjoint=self.disjoint, | |||
| ) | |||
|
|
|||
| # geometry.Transform(transformation_to_rhino(self.worldtransformation)) | |||
| geometry.Transform(transformation_to_rhino(self.worldtransformation)) | |||
There was a problem hiding this comment.
This is weird, do you know why it was commented out and why it's needed now?
There was a problem hiding this comment.
scene objects can have a transformation. before it was applied through the vertex caching. now it is done explicitly here...
| self._guids_vertices = guids | ||
| self._guid_vertex = dict(zip(guids, vertices)) | ||
|
|
||
| self._guids += guids |
There was a problem hiding this comment.
This causes an error if the scene has not been drawn at least once because self._guids is None. Maybe should be initialized on __init__
There was a problem hiding this comment.
ha weird maybe i forgot to push this
There was a problem hiding this comment.
if you have a scene.draw() before trying something like scene_object.draw_faces() it works, but I guess it's a valid case to just draw the faces before drawing the full scene:
import compas
from compas.datastructures import Mesh
from compas.scene import Scene
from compas.colors import Color
# Load a mesh from a sample OBJ file
mesh = Mesh.from_obj(compas.get("tubemesh.obj"))
scene = Scene()
sp = scene.add(mesh)
sp.draw_faces() # this line fails if draw() is after
# drawing scene after faces causes the exception
# scene.draw()
ok. i can put those back. it is actually not crucial for the PR |
|
will review and push new soon |
|
LGTM! but will leave @gonzalocasas to say final yes : ) |
A few backwards compatible tweaks to the scene functionality, especially refinement of how things are cleared from the viz context...
What type of change is this?
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.CHANGELOG.mdfile in theUnreleasedsection under the most fitting heading (e.g.Added,Changed,Removed).invoke test).invoke lint).compas.datastructures.Mesh.