feature/view-setting-config #208

ZacZhangzhuo · 2023-12-13T11:38:18Z

What type of change is this?

Added view_angle and camera in the config. This should close #207 .

Thanks @tomvanmele for the proposal.

Licini

Can you also add an example script.

Licini · 2023-12-13T14:58:43Z

src/compas_view2/views/view.py

-    RIGHT = 2
-    TOP = 3
-    PERSPECTIVE = 4
+    VIEW_ANGLES = {"CUSTOM": 0, "FRONT": 1, "RIGHT": 2, "TOP": 3, "PERSPECTIVE": 4}


What would be the benefit to change these numeration to dict?

Thanks for the review!

The reason is for the readability of the config file: the original version would only allow us to put numbers (i,e. "view_angle":1 ) in the config. While using dict would allow us to indicate the key: "view_angle": "TOP".

Licini · 2023-12-13T14:59:53Z

src/compas_view2/app/config_default.json

@@ -14,8 +14,17 @@
    "view": {
        "show_grid": true,
        "view_mode": "shaded",
+        "view_angle": "CUSTOM",


angle suggest things like Euler angle, any other name?

Also I think we should use a custom setting here in the default config. It should be something minimal and simple.

angle suggest things like Euler angle, any other name?

agree, I was having trouble with naming this as well. In rhino it is called view setting which is a bit general as well.

how about view_port ? like "view_port":"TOP"

Also I think we should use a custom setting here in the default config. It should be something minimal and simple.

sorry I meant we shouldn't ............. I miss typed

"view_port" is better, let's use that

ZacZhangzhuo · 2023-12-14T10:51:56Z

requirements-dev.txt

pin the sphinx version, related to compas-dev/compas_invocations@067a873

ZacZhangzhuo · 2023-12-14T10:52:43Z

src/compas_view2/app/config_default.json

Also I think we should use a custom setting here in the default config. It should be something minimal and simple.

the default value is already custom.

ZacZhangzhuo · 2023-12-14T11:46:32Z

ZacZhangzhuo · 2023-12-15T08:20:12Z

Wait, I should put out a better solution that also solves #209 . I convert it into draft and will work on it.

maybe do you have any clue about the break point of the camera? sometimes it becomes blank and not useable. there might be some trick in setting the params. @Licini

Licini · 2023-12-15T11:45:57Z

@ZacZhangzhuo We should also allow using programmable API to achieve this besides config.
Something like

viewer = App()
...
viewer.camera.top_view()
viewer.show()

or

viewer = App(view_port="top")
...
viewer.show()

…config

ZacZhangzhuo · 2023-12-15T18:28:12Z

Hi @Licini , I updated the solution and it is ready to be reviewed:

no CUSTOM option, but in PERSPECTIVE you can define the target, position and any other params.
The default config is minimal: PERSPECTIVE, looking at the origin.
Params for TOP, FRONT, and RIGHT are also configurable, like the target. Except for the position

docs/examples/control/example_camera_config.rst

docs/tutorials/tutorial_basics.rst

docs/tutorials/tutorial_configuration.rst

docs/tutorials/tutorial_software_concepts.rst

requirements-dev.txt

tomvanmele · 2023-12-16T10:22:06Z

src/compas_view2/app/config_default.json

@@ -14,8 +14,17 @@
    "view": {
        "show_grid": true,
        "view_mode": "shaded",
+        "view_port": "PERSPECTIVE",


personally, i would not add an underscore in keys like these. you could consider "viewmode" and "viewport" as one word. "show_grid" is different in that sense...

Yes. I didn't find the naming conversions for the configuration / .jason file so I named it this way. All the items in this file, including some long words, like enable_propertyform, flow_view_size, zoom_selected, view_perspective, background_color, and selection_color, then need to be changed. Shall we decide fixing them all?

We could create a separate PR/Issue for this

src/compas_view2/app/controller.py

src/compas_view2/views/view.py

tomvanmele · 2023-12-16T10:31:38Z

src/compas_view2/views/view.py

        self.shader_model = None
        self.app = app
        self.color = view_config["background_color"]
        self.mode = view_config["view_mode"]
        self.selection_color = view_config["selection_color"]
        self.show_grid = view_config["show_grid"]
-        self.camera = Camera(self)
+        self.camera = Camera(self, **view_config["camera"])


also to my point earlier, view_config could be interpreted as "viewing the config" (action + subject). it is clearer what is meant when viewconfig is used, in my opinion...

Licini · 2023-12-16T17:02:13Z

@ZacZhangzhuo We should also allow using programmable API to achieve this besides config. Something like
viewer = App()
...
viewer.camera.top_view()
viewer.show()
or
viewer = App(view_port="top")
...
viewer.show()

Almost there, as Tom mentioned, let's make view_port to viewport and view_mode to viewmode. And please add kwarg to App init api e.g. App(viewport="Top")

ZacZhangzhuo · 2023-12-16T17:55:48Z

changed, you can use the below code to exam:

viewer = App(viewport="front")
viewer.show()

since this is a small update, I have not put an example yet. but the doc is updated in the tutorial_configuration:

2. In-code definition which overwrites the configuration:

::

    >>> from compas_view2.app import App
    >>> viewer = App(viewmode="lighted", viewport = "top", enable_sceneform=True, enable_propertyform=True, enable_sidebar=True, width=2000, height=1000)

let me know if you think we should add an extra example file.

also, the underscores of other options in the config:

like enable_propertyform, flow_view_size, zoom_selected, view_perspective, background_color, and selection_color,

are not changed. let me know if you want to change them too.

docs/examples/control/example_camera_config.rst

docs/tutorials/tutorial_basics.rst

tomvanmele · 2023-12-17T10:05:34Z

sorry i approved this by accident. will leave the approval up to @Licini

tomvanmele · 2023-12-17T12:59:12Z

@ZacZhangzhuo if you bring in the latest changes from main the docs will build a lot faster...

ZacZhangzhuo · 2023-12-17T13:07:43Z

@ZacZhangzhuo if you bring in the latest changes from main the docs will build a lot faster...

thanks for the update change!

ZacZhangzhuo and others added 2 commits December 13, 2023 12:36

push

d1a50f4

Merge branch 'main' into feature/view-setting-config

c1ee6c7

ZacZhangzhuo requested a review from Licini December 13, 2023 12:26

Licini requested changes Dec 13, 2023

View reviewed changes

ZacZhangzhuo added 2 commits December 14, 2023 11:49

update with the example

e9246da

changelog

df80ec2

ZacZhangzhuo commented Dec 14, 2023

View reviewed changes

requirements-dev.txt Outdated

Copy link

Collaborator Author

ZacZhangzhuo Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pin the sphinx version, related to compas-dev/compas_invocations@067a873

ZacZhangzhuo commented Dec 14, 2023

View reviewed changes

ZacZhangzhuo requested a review from Licini December 14, 2023 10:53

ZacZhangzhuo self-assigned this Dec 14, 2023

ZacZhangzhuo added 2 commits December 14, 2023 12:55

lint

513fcfd

fix the lint

beeeab5

ZacZhangzhuo marked this pull request as draft December 15, 2023 08:12

ZacZhangzhuo and others added 3 commits December 15, 2023 18:21

Merge remote-tracking branch 'origin/main' into feature/view-setting-…

dd5c768

…config

update the better solution

715eaeb

Merge branch 'main' into feature/view-setting-config

9a1d911

ZacZhangzhuo marked this pull request as ready for review December 15, 2023 18:31

tomvanmele reviewed Dec 16, 2023

View reviewed changes

Fix rst line =, change VIEW_ANGLES to VIEW_PORTS

644e72d

ZacZhangzhuo closed this Dec 16, 2023

ZacZhangzhuo deleted the feature/view-setting-config branch December 16, 2023 16:07

ZacZhangzhuo restored the feature/view-setting-config branch December 16, 2023 16:08

ZacZhangzhuo reopened this Dec 16, 2023

ZacZhangzhuo added 2 commits December 16, 2023 18:49

naming change, viewmode, viewport, etc.

4fc2f70

changelog

4c5cda7

ZacZhangzhuo added 2 commits December 16, 2023 18:58

VIEW_PORTS to VIEWPORTS

8175e4b

patch

ee73940

tomvanmele approved these changes Dec 17, 2023

View reviewed changes

docs/examples/control/example_camera_config.rst Outdated Show resolved Hide resolved

docs/tutorials/tutorial_basics.rst Outdated Show resolved Hide resolved

tomvanmele self-requested a review December 17, 2023 10:01

ZacZhangzhuo and others added 2 commits December 17, 2023 11:05

title lines fix

4b704fa

Merge branch 'main' into feature/view-setting-config

1e1a2e9

Merge branch 'main' into feature/view-setting-config

ec9abb7

Licini approved these changes Dec 17, 2023

View reviewed changes

Licini merged commit 349b0ba into compas-dev:main Dec 17, 2023
11 checks passed

ZacZhangzhuo deleted the feature/view-setting-config branch December 18, 2023 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature/view-setting-config #208

feature/view-setting-config #208

ZacZhangzhuo commented Dec 13, 2023

Licini left a comment

Licini Dec 13, 2023

ZacZhangzhuo Dec 14, 2023

Licini Dec 13, 2023

Licini Dec 13, 2023

ZacZhangzhuo Dec 14, 2023 •

edited

Loading

ZacZhangzhuo Dec 14, 2023 •

edited by Licini

Loading

Licini Dec 14, 2023

Licini Dec 14, 2023

ZacZhangzhuo Dec 14, 2023

ZacZhangzhuo Dec 14, 2023 •

edited

Loading

ZacZhangzhuo commented Dec 14, 2023

ZacZhangzhuo commented Dec 15, 2023 •

edited

Loading

Licini commented Dec 15, 2023 •

edited

Loading

ZacZhangzhuo commented Dec 15, 2023 •

edited

Loading

tomvanmele Dec 16, 2023

ZacZhangzhuo Dec 16, 2023

tomvanmele Dec 16, 2023

Licini commented Dec 16, 2023

ZacZhangzhuo commented Dec 16, 2023

tomvanmele commented Dec 17, 2023

tomvanmele commented Dec 17, 2023

ZacZhangzhuo commented Dec 17, 2023

feature/view-setting-config #208

feature/view-setting-config #208

Conversation

ZacZhangzhuo commented Dec 13, 2023

What type of change is this?

Licini left a 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

ZacZhangzhuo Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

ZacZhangzhuo Dec 14, 2023 • edited by Licini 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

ZacZhangzhuo Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

ZacZhangzhuo commented Dec 14, 2023

ZacZhangzhuo commented Dec 15, 2023 • edited Loading

Licini commented Dec 15, 2023 • edited Loading

ZacZhangzhuo commented Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Licini commented Dec 16, 2023

ZacZhangzhuo commented Dec 16, 2023

tomvanmele commented Dec 17, 2023

tomvanmele commented Dec 17, 2023

ZacZhangzhuo commented Dec 17, 2023

ZacZhangzhuo Dec 14, 2023 •

edited

Loading

ZacZhangzhuo Dec 14, 2023 •

edited by Licini

Loading

ZacZhangzhuo Dec 14, 2023 •

edited

Loading

ZacZhangzhuo commented Dec 15, 2023 •

edited

Loading

Licini commented Dec 15, 2023 •

edited

Loading

ZacZhangzhuo commented Dec 15, 2023 •

edited

Loading