Feature/viewer-widget #8

ZacZhangzhuo · 2023-12-17T16:23:50Z

What type of change is this?

Basic Viewer buildup with the widgets:
- viewer lunchable. The main widget and the other associated widgets can be shown on the screen.
- test files added for checking.
- doc file cleanup for sphinx-compas2-theme compatibility.
- tutorial docs added.
- some other small patches.

Checklist

Put an x in 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.

I added a line to the CHANGELOG.md file in the Unreleased section under the most fitting heading (e.g. Added, Changed, Removed).
I ran all tests on my computer and it's all green (i.e. invoke test).
I ran lint on my computer and there are no errors (i.e. invoke lint).
I added new functions/classes and made them available on a second-level import, e.g. compas.datastructures.Mesh.
I have added tests that prove my fix is effective or that my feature works.
I have added necessary documentation (if appropriate)

ZacZhangzhuo · 2023-12-17T16:26:22Z

docs/_static/compas.css

static icons and css clean up. the config.py is also updated based on Tom's template.

ZacZhangzhuo · 2023-12-17T16:30:47Z

src/compas_viewer/viewer.py

+    # ==========================================================================
+    # Messages
+    # ==========================================================================
+    def _about(self) -> None:


naming this _about because the name about is occupied by the about: str from the config file.

I was careful about the _ for private functions and attributes. most of them I produced are private, so they are all with the _ prefix

ZacZhangzhuo · 2023-12-17T16:31:39Z

temp.py

this random file was accidentally uploaded last time. remove now

src/compas_viewer/configurations/controller_config.py

src/compas_viewer/viewer.py

tomvanmele · 2023-12-17T16:36:52Z

src/compas_viewer/viewer.py

+        config: Optional[ViewerConfigData] = None,
+    ) -> None:
+        # custom or default config
+        config = config or Viewer.from_default().data


the use from_default like this doesn't make sense to me.
"from" functions are supposed to be alternative constructors.
they can be used as an alternative to the normal constructor, not inside of it.

yes. let me write as config = config or ViewerConfig.from_default().data which makes more sense.

tomvanmele · 2023-12-17T16:37:56Z

src/compas_viewer/viewer.py

+ICONS = Path(Path(__file__).parent, "_static", "icons")
+
+
+class Viewer(ViewerConfig):


why does the viewer extend the viewer configuration?
should the configuration not be an attribute of the viewer?

maybe we should find a better name for ViewerConfig since I understand making the viewer inherited from its config doesn't make sense.. but the idea is:

inheritance tree: compas.Data -> compas_viewer.Config -> compas_viewer.ViewerConfig -> compas_viewer.View so the needed config is safely checked before initialising the Viewer, and we don't have to assign every config attribute into the Viewer. The Viewer naturally has the attributes like width at the beginning.

In the past we the View was inherited from the Qt like class View(QtWidgets.QOpenGLWidget): , which is a bit indirect for other operations.

things like an OpenGL view should not be based on the data framework

tomvanmele · 2023-12-17T16:38:39Z

src/compas_viewer/viewer.py

+    # ==========================================================================
+    # Messages
+    # ==========================================================================
+    def _about(self) -> None:


add blank line and remove the underscore (since this seems to be a public method: it is used in one of the examples)

yes you are right. but

naming this _about because the name about is occupied by the about: str from the config file.

any suggestion on solving this?

it cannot be that the config file is in this namespace since the viewer should not inherit from config

i think this can now just be "about"

ZacZhangzhuo · 2023-12-17T17:15:24Z

pytest locally sucssful but in the github action:

tests/test_placeholder.py .                                              [ 50%]
Fatal Python error: Aborted

Current thread 0x00007f48d8437b80 (most recent call first):
  File "/home/runner/work/compas_viewer/compas_viewer/src/compas_viewer/viewer.py", line 105 in _init
  File "/home/runner/work/compas_viewer/compas_viewer/src/compas_viewer/viewer.py", line 89 in __init__
  File "/home/runner/work/compas_viewer/compas_viewer/tests/test_viewer.py", line 12 in test_default_config
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/python.py", line 194 in pytest_pyfunc_call
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_callers.py", line 77 in _multicall
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_manager.py", line 115 in _hookexec
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_hooks.py", line 493 in __call__
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/python.py", line 1[792](https://github.com/compas-dev/compas_viewer/actions/runs/7239911557/job/19722325003?pr=8#step:2:807) in runtest
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/runner.py", line 169 in pytest_runtest_call
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_callers.py", line 77 in _multicall
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_manager.py", line 115 in _hookexec
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_hooks.py", line 493 in __call__
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/runner.py", line 262 in <lambda>
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/runner.py", line 341 in from_call
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/runner.py", line 261 in call_runtest_hook
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/runner.py", line 222 in call_and_report
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/runner.py", line 133 in runtestprotocol
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/runner.py", line 114 in pytest_runtest_protocol
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_callers.py", line 77 in _multicall
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_manager.py", line 115 in _hookexec
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_hooks.py", line 493 in __call__
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/main.py", line 350 in pytest_runtestloop
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_callers.py", line 77 in _multicall
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_manager.py", line 115 in _hookexec
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_hooks.py", line 493 in __call__
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/main.py", line 325 in _main
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/main.py", line 271 in wrap_session
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/main.py", line 318 in pytest_cmdline_main
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_callers.py", line 77 in _multicall
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_manager.py", line 115 in _hookexec
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/pluggy/_hooks.py", line 493 in __call__
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/config/__init__.py", line 169 in main
  File "/opt/hostedtoolcache/Python/3.8.18/x64/lib/python3.8/site-packages/_pytest/config/__init__.py", line 192 in console_main
  File "/opt/hostedtoolcache/Python/3.8.18/x64/bin/pytest", line 8 in <module>
tests/test_viewer.py 
Error: Process completed with exit code 250.

any suggestion?

should I write less test files but doc examples to exam... ?

tomvanmele · 2023-12-17T17:55:05Z

for me the test works locally, but the user has to close the viewer window to stop the test so i don't know how this would work in a workflow. i don't think this is needed right now...

tomvanmele · 2023-12-17T17:59:22Z

also, i would still use an environment with a conda-based setup for this repo, especially to make it cross-platform. for me the following environment works well and doesn't take much time to resolve... (you may need to tweak it for windows)

name: viewer-dev
channels:
  - conda-forge
dependencies:
  - python=3.9
  - pip
  - freetype-py
  - textdistance
  - pyside2
  - pyopengl
  - qtpy
  - pip:
    - -r requirements-dev.txt

and compas 2 is needed as well of course...

ZacZhangzhuo · 2023-12-17T18:07:32Z

for me the test works locally, but the user has to close the viewer window to stop the test so i don't know how this would work in a workflow. i don't think this is needed right now...

that is because pytest intelligently test examples:

057     Examples
058     -------
059     >>> from compas_viewer import Viewer
060     >>> viewer = Viewer()
061     >>> viewer.show()

I remove the example for now

tomvanmele · 2023-12-17T18:10:29Z

you can add # doctest: +SKIP for stuff like this

tomvanmele · 2023-12-17T18:13:08Z

also, i would still use an environment with a conda-based setup for this repo, especially to make it cross-platform. for me the following environment works well and doesn't take much time to resolve... (you may need to tweak it for windows)
name: viewer-dev
channels:
  - conda-forge
dependencies:
  - python=3.9
  - pip
  - freetype-py
  - textdistance
  - pyside2
  - pyopengl
  - qtpy
  - pip:
    - -r requirements-dev.txt
and compas 2 is needed as well of course...

but if it worked with the pip install until now, just leave it, we can have a look at this later...

tomvanmele · 2023-12-17T18:16:25Z

src/compas_viewer/viewer.py

+    Examples
+    -------
+    >>> from compas_viewer import Viewer
+    >>> viewer = Viewer()


you need to skip this as well, because it sets a lot of stuff in motion that is otherwise never properly stopped...

ZacZhangzhuo · 2023-12-17T18:32:44Z

comment summary:

This PR is ready to be merged if we are happy with these three questions:
#8 (comment) (changed from Viewer.from_default().data into ViewerConfig.from_default().data
#8 (comment) (changed, Viewer no longer inherent from ViewerConfig)
#8 (comment) (unchanged, need naming or other suggestions)

@Licini

tomvanmele · 2023-12-17T22:44:14Z

src/compas_viewer/viewer.py

+    # ==========================================================================
+    # Messages
+    # ==========================================================================
+    def _about(self) -> None:


i think this can now just be "about"

src/compas_viewer/viewer.py

ZacZhangzhuo added 4 commits December 8, 2023 12:30

stash

7be86c8

push changes

96fa354

push changes

f44d218

push

fb26c50

ZacZhangzhuo commented Dec 17, 2023

View reviewed changes

temp.py Outdated

Copy link

Collaborator Author

ZacZhangzhuo Dec 17, 2023

Choose a reason for hiding this comment

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

this random file was accidentally uploaded last time. remove now

ZacZhangzhuo added 2 commits December 17, 2023 17:33

Update requirements-dev.txt

d6beb56

lint patch

1ffea99

tomvanmele requested changes Dec 17, 2023

View reviewed changes

ZacZhangzhuo added 2 commits December 17, 2023 18:12

update based on the comments

164d414

patch

3155c5a

ZacZhangzhuo requested a review from tomvanmele December 17, 2023 17:16

ZacZhangzhuo self-assigned this Dec 17, 2023

remove example

42fd95a

skip one doctest

8b4e61b

tomvanmele reviewed Dec 17, 2023

View reviewed changes

ZacZhangzhuo added 3 commits December 17, 2023 19:17

remove the viewer test

f8c3599

add test cases

6633a50

Refactor viewer configuration tests

d050a78

Refactor Viewer class configuration handling

6a2982f

tomvanmele reviewed Dec 17, 2023

View reviewed changes

Update viewer.py

ab42f47

Update messages.rst

babd20f

ZacZhangzhuo requested a review from tomvanmele December 18, 2023 12:04

tomvanmele approved these changes Dec 18, 2023

View reviewed changes

ZacZhangzhuo merged commit e8c0ffb into main Dec 19, 2023

ZacZhangzhuo deleted the feature/render branch December 19, 2023 15:00

ZacZhangzhuo mentioned this pull request Dec 19, 2023

Doc build error #6

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/viewer-widget #8

Feature/viewer-widget #8

ZacZhangzhuo commented Dec 17, 2023 •

edited

Loading

ZacZhangzhuo Dec 17, 2023

ZacZhangzhuo Dec 17, 2023

ZacZhangzhuo Dec 17, 2023

tomvanmele Dec 17, 2023

ZacZhangzhuo Dec 17, 2023

tomvanmele Dec 17, 2023

ZacZhangzhuo Dec 17, 2023 •

edited

Loading

tomvanmele Dec 17, 2023

tomvanmele Dec 17, 2023

ZacZhangzhuo Dec 17, 2023

tomvanmele Dec 17, 2023 •

edited

Loading

tomvanmele Dec 17, 2023

ZacZhangzhuo commented Dec 17, 2023 •

edited

Loading

tomvanmele commented Dec 17, 2023

tomvanmele commented Dec 17, 2023 •

edited

Loading

ZacZhangzhuo commented Dec 17, 2023

tomvanmele commented Dec 17, 2023

tomvanmele commented Dec 17, 2023

tomvanmele Dec 17, 2023

ZacZhangzhuo commented Dec 17, 2023 •

edited

Loading

tomvanmele Dec 17, 2023

		ICONS = Path(Path(__file__).parent, "_static", "icons")


		class Viewer(ViewerConfig):

Feature/viewer-widget #8

Feature/viewer-widget #8

Conversation

ZacZhangzhuo commented Dec 17, 2023 • edited Loading

What type of change is this?

Checklist

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

ZacZhangzhuo Dec 17, 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

tomvanmele Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacZhangzhuo commented Dec 17, 2023 • edited Loading

tomvanmele commented Dec 17, 2023

tomvanmele commented Dec 17, 2023 • edited Loading

ZacZhangzhuo commented Dec 17, 2023

tomvanmele commented Dec 17, 2023

tomvanmele commented Dec 17, 2023

Choose a reason for hiding this comment

ZacZhangzhuo commented Dec 17, 2023 • edited Loading

Choose a reason for hiding this comment

ZacZhangzhuo commented Dec 17, 2023 •

edited

Loading

ZacZhangzhuo Dec 17, 2023 •

edited

Loading

tomvanmele Dec 17, 2023 •

edited

Loading

ZacZhangzhuo commented Dec 17, 2023 •

edited

Loading

tomvanmele commented Dec 17, 2023 •

edited

Loading

ZacZhangzhuo commented Dec 17, 2023 •

edited

Loading