Do not reset the grid and axe when the scene is empty #83

hbcarlos · 2022-12-23T11:49:43Z

Fixes #66

Should we stop re-setting the camera too? See the screencast:

Grabacion.de.pantalla.2022-12-23.a.las.12.46.37.mov

github-actions · 2022-12-23T11:49:54Z

👈 Launch a Binder on branch hbcarlos/jupytercad/fix_66

martinRenou · 2023-01-02T15:19:48Z

I think there are two cases to consider here when there is only one object left:

when we hide the last object: we should not reset any of the camera/grid/axes settings
when we remove the last object: we should reset the camera/grid and axes settings

The problem in the current implementation is that you cannot spot in which of those two cases you are under the _shapeToMesh method.

One way to fix it would be to change the behavior when hiding an object. I believe hiding an object should not trigger a call to _shapeToMesh. Instead, it should simply take the ThreeJS Object3D(s) it refers to and set object.visible = false (see this). This way we don't go through this _shapeToMesh logic of recreating all the THREE.BufferGeometry object (which is far from being ideal because we recreate buffers and re-send them to the GPU while it was actually not needed 😢) and resetting the camera/grid/axes when hiding/showing objects: problem fixed!

trungleduc · 2023-01-03T13:51:01Z

I think there are two cases to consider here when there is only one object left:

when we hide the last object: we should not reset any of the camera/grid/axes settings

when we remove the last object: we should reset the camera/grid and axes settings

The problem in the current implementation is that you cannot spot in which of those two cases you are under the _shapeToMesh method.

While working with the 2D editor, I realize that we should have a unit system for the length and stick with it. Users need to handle by themself the camera in case of creating something too big. In this case, we can have a static grid/axes.

One way to fix it would be to change the behavior when hiding an object. I believe hiding an object should not trigger a call to _shapeToMesh. Instead, it should simply take the ThreeJS Object3D(s) it refers to and set object.visible = false (see this). This way we don't go through this _shapeToMesh logic of recreating all the THREE.BufferGeometry object (which is far from being ideal because we recreate buffers and re-send them to the GPU while it was actually not needed 😢) and resetting the camera/grid/axes when hiding/showing objects: problem fixed!

I thought about showing and hiding objects by using threejs at the start. But since visible is a property of the document model and we need to update it on the FCStd file, I decide to treat it as other cases of modifying the document. So we might need to add some logic to filer out the update of visible property from the shared model change signal.

hbcarlos · 2023-01-09T16:54:35Z

One way to fix it would be to change the behavior when hiding an object. I believe hiding an object should not trigger a call to _shapeToMesh.

So we might need to add some logic to filer out the update of visible property from the shared model change signal.

I added the logic to filter the object visibility change and modify the view instead of re-rendering everything.

trungleduc · 2023-01-10T09:05:46Z

Thanks @hbcarlos, could you rebase this PR to pick up the new project structure? We should not commit the output of the opencascade build

martinRenou · 2023-01-10T10:14:11Z

The CI failure might be legit?

hbcarlos · 2023-01-10T10:19:44Z

The CI failure might be legit?

No, is not. The extension is not activated

trungleduc · 2023-01-10T10:21:33Z

The CI failure might be legit?

No, is not. The extension is not activated

so we need your fix on ydoc?

martinRenou · 2023-01-10T10:45:13Z

src/mainview.tsx

+      if (!visible) {
+        this._postMessage({
+          action: WorkerAction.LOAD_FILE,
+          payload: {
+            fileName: this._context.path,
+            content: this._model.getContent()
+          }
+        });
+      }


I'm not sure I understand this part. What is it doing?

I think the variable visible should be named visibleChanged.
But I'm wondering what is the behavior of this function if I have both visible and other keys in the change.objectChange list. We should send the message to the worker to re-render the scene in this case.

I'm not sure it matters much. I'm currently working on the guidata options and having the visible property there. So this method will actually be triggered on an sharedOptionChange event, not an object change, I'm rebasing my PR against Carlos's branch to implement this.

We have both visible properties in the document and the GUI document. When users click on the visibility icon in the tree view, which visible property we should update? If it's both, then the viewer will get both sharedOptionChange and sharedObjectChange events.

I believe it should be the visible property from the GUI data that we should change.

Yes it can :)

So let's get #91 in first then we will adapt this PR?

I'd prefer merging #83 first actually, because I already rebased my work on this locally

ok, I will try to fix the integration tests before merging this PR.

But I'm wondering what is the behavior of this function if I have both visible and other keys in the change.objectChange list. We should send the message to the worker to re-render the scene in this case.

When changing the visibility of the object from the tree view the transaction has only one change, the visible key. I think is not possible to have an event that changes multiple keys when changing the visibility.

martinRenou · 2023-01-10T14:03:39Z

With this PR, the grid helper is too small for big objects, making it useless (it's barely visible on this screenshot):

I feel like we should remove the grid helper completely and display 3D axes that are not bound to the 3D world projection like freecad does:

This way the 3D axes are always of the same size/position and don't depend on the camera position

martinRenou · 2023-01-10T14:07:42Z

So maybe this PR should actually remove the grid helper?

trungleduc · 2023-01-10T14:41:26Z

So maybe this PR should actually remove the grid helper?

I'm in favor of removing the grid too. and for the camera, I think we should only scale it once when users open the document.

hbcarlos · 2023-01-10T17:41:39Z

@martinRenou @trungleduc I will open a new PR to make the grid configurable by the user.

The test in this PR fail because of the grid, I would like to move on and fix the grid in another PR.

trungleduc · 2023-01-10T20:24:23Z

Thanks @hbcarlos !

hbcarlos added the bug Something isn't working label Dec 23, 2022

hbcarlos self-assigned this Dec 23, 2022

hbcarlos requested review from martinRenou and trungleduc and removed request for martinRenou December 23, 2022 11:52

hbcarlos added 2 commits January 9, 2023 17:27

Do not reset the grid and axe when the scene is empty

2a71816

Prevent rendering when visibility changes

06c4edb

hbcarlos force-pushed the fix_66 branch from 5295efd to abc4401 Compare January 9, 2023 16:35

hbcarlos added 2 commits January 10, 2023 10:08

Clean up rebase

d7a382d

Remove opencascade

ba4fa56

hbcarlos force-pushed the fix_66 branch from 8c51b14 to ba4fa56 Compare January 10, 2023 09:09

trungleduc approved these changes Jan 10, 2023

View reviewed changes

martinRenou reviewed Jan 10, 2023

View reviewed changes

hbcarlos added 2 commits January 10, 2023 15:29

Update dependencies

89e623c

Lint

6f4e9fe

hbcarlos added 2 commits January 10, 2023 16:57

Update model

854dfc4

Lint

8fe295d

trungleduc linked an issue Jan 10, 2023 that may be closed by this pull request

JupyterCad does not work with JupyterLab==4.0.0a32 and @jupyter/ydoc==0.2.2 #92

Closed

hbcarlos added 2 commits January 10, 2023 18:19

Fixes test announcement dialog

9ee7e4b

Lint

71dfabf

trungleduc merged commit 900ab4c into jupytercad:main Jan 10, 2023

hbcarlos deleted the fix_66 branch January 11, 2023 07:29

martinRenou mentioned this pull request Jan 11, 2023

Toolbar for configuring the grid and axes #94

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not reset the grid and axe when the scene is empty #83

Do not reset the grid and axe when the scene is empty #83

hbcarlos commented Dec 23, 2022 •

edited

Loading

github-actions bot commented Dec 23, 2022

martinRenou commented Jan 2, 2023

trungleduc commented Jan 3, 2023

hbcarlos commented Jan 9, 2023

trungleduc commented Jan 10, 2023

martinRenou commented Jan 10, 2023

hbcarlos commented Jan 10, 2023

trungleduc commented Jan 10, 2023

martinRenou Jan 10, 2023

trungleduc Jan 10, 2023

martinRenou Jan 10, 2023 •

edited

Loading

trungleduc Jan 10, 2023 •

edited

Loading

martinRenou Jan 10, 2023

martinRenou Jan 10, 2023

trungleduc Jan 10, 2023

martinRenou Jan 10, 2023

trungleduc Jan 10, 2023

hbcarlos Jan 10, 2023

martinRenou commented Jan 10, 2023 •

edited

Loading

martinRenou commented Jan 10, 2023

trungleduc commented Jan 10, 2023

hbcarlos commented Jan 10, 2023

trungleduc commented Jan 10, 2023

Do not reset the grid and axe when the scene is empty #83

Do not reset the grid and axe when the scene is empty #83

Conversation

hbcarlos commented Dec 23, 2022 • edited Loading

github-actions bot commented Dec 23, 2022

martinRenou commented Jan 2, 2023

trungleduc commented Jan 3, 2023

hbcarlos commented Jan 9, 2023

trungleduc commented Jan 10, 2023

martinRenou commented Jan 10, 2023

hbcarlos commented Jan 10, 2023

trungleduc commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinRenou Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

trungleduc Jan 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinRenou commented Jan 10, 2023 • edited Loading

martinRenou commented Jan 10, 2023

trungleduc commented Jan 10, 2023

hbcarlos commented Jan 10, 2023

trungleduc commented Jan 10, 2023

hbcarlos commented Dec 23, 2022 •

edited

Loading

martinRenou Jan 10, 2023 •

edited

Loading

trungleduc Jan 10, 2023 •

edited

Loading

martinRenou commented Jan 10, 2023 •

edited

Loading