Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added click-boxes of show and locked #169

Merged
merged 23 commits into from
Jun 26, 2024
Merged

Added click-boxes of show and locked #169

merged 23 commits into from
Jun 26, 2024

Conversation

PingHsunTsai
Copy link
Collaborator

@PingHsunTsai PingHsunTsai commented May 27, 2024

#146

What type of change is this?

  • * Added click-boxes of show and locked for compas_viewer.components.SceneForm.
  • * Added observer pattern to of show and locked for compas_viewer.Scene.scene.
  • * Added Time Debounce pattern to of show and locked for compas_viewer.Scene.scene.

Screenshot from 2024-05-27 17-48-08

@PingHsunTsai PingHsunTsai self-assigned this May 27, 2024
@Licini
Copy link
Collaborator

Licini commented Jun 7, 2024

This is nice, only thing is that, I feel is_locked is not necessary to be shown here, what you think? @tomvanmele

@Licini
Copy link
Collaborator

Licini commented Jun 7, 2024

I don't know even is_locked is useful at all in any cases, as this is not aiming on being a CAD environment that needs to do all kinds editing

@Licini Licini mentioned this pull request Jun 7, 2024
1 task
@tomvanmele
Copy link
Member

I don't know even is_locked is useful at all in any cases, as this is not aiming on being a CAD environment that needs to do all kinds editing

yes, let's get rid of is_locked completely...

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 219 to 232
@property
def is_locked(self):
return self._is_locked

@is_locked.setter
def is_locked(self, value: bool):
self._is_locked = value
if value:
self.is_selected = False
# scene parent
self.scene.instance_colors.pop(self.instance_color.rgb255)
else:
self.scene.instance_colors[self.instance_color.rgb255] = self

Copy link
Collaborator

Choose a reason for hiding this comment

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

We will be removing is_locked all together, but will be another PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay!

@PingHsunTsai PingHsunTsai requested a review from Licini June 12, 2024 09:07
src/compas_viewer/config.py Outdated Show resolved Hide resolved
@PingHsunTsai PingHsunTsai mentioned this pull request Jun 14, 2024
1 task
@PingHsunTsai PingHsunTsai requested a review from Licini June 14, 2024 14:30
src/compas_viewer/config.py Outdated Show resolved Hide resolved
src/compas_viewer/components/sceneform.py Outdated Show resolved Hide resolved
src/compas_viewer/components/sceneform.py Outdated Show resolved Hide resolved
@PingHsunTsai PingHsunTsai requested a review from Licini June 25, 2024 14:40
CHANGELOG.md Outdated
Comment on lines 25 to 27
* Added click-boxes of `show` for `compas_viewer.components.SceneForm`.
* Added observer pattern to of `show` for `compas_viewer.Scene.scene`.
* Added Time Debounce pattern to of `show` for `compas_viewer.Scene.scene`.
Copy link
Member

Choose a reason for hiding this comment

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

please convert this into understandable sentences...

@@ -206,7 +207,8 @@ def __init__(
self.linewidth = 1.0 if linewidth is None else linewidth
self.opacity = 1.0 if opacity is None else opacity
self.doublesided = True if doublesided is None else doublesided
self.is_visible = True if is_visiable is None else is_visiable
self.show = True if is_visiable is None else is_visiable
Copy link
Member

Choose a reason for hiding this comment

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

is_visiable => is_visible

Copy link
Member

Choose a reason for hiding this comment

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

btw, i am not a fan of self.show. i think it should be self.is_visible. self.show sounds like a method...

Copy link
Collaborator

Choose a reason for hiding this comment

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

show comes from compas core, which I suggested a while ago for the simplicity, but I agree indeed it can feel like a method. If we decide to go back to is_visible, I would suggest we make this change in compas core first then update here

@@ -206,7 +207,8 @@ def __init__(
self.linewidth = 1.0 if linewidth is None else linewidth
self.opacity = 1.0 if opacity is None else opacity
self.doublesided = True if doublesided is None else doublesided
self.is_visible = True if is_visiable is None else is_visiable
self.show = True if is_visiable is None else is_visiable
self._is_locked = False if is_locked is None else is_locked
Copy link
Member

Choose a reason for hiding this comment

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

were we not going to remove this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes we are, but it would be better to do that in another PR, cuz this attribute is a bit everywhere, this one here is added temporarily to make the form work

@@ -70,10 +81,35 @@ def __init__(self, name: str = "ViewerScene", context: str = "Viewer"):

# Primitive
self.objects: list[ViewerSceneObject]
Copy link
Member

Choose a reason for hiding this comment

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

btw, this does not initialize the attribute. it just adds a type annotation. if you try to access this attribute an error will be thrown. it should be self.objects: list[ViewerSceneObject] = []

Copy link
Collaborator

Choose a reason for hiding this comment

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

objects is inherent from the parent class, I believe this was only for additionally type-hinting the object will be ViewerScenceObject instead of the base SceneObject

Comment on lines 102 to 112
@property
def observers(self):
new_observers = [
self.viewer.renderer,
self.viewer.ui.sidebar,
]

for observer in new_observers:
self._observers.add(observer)

return list(self._observers)
Copy link
Member

Choose a reason for hiding this comment

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

this looks a bit fishy...

Attributes
----------
observers : set
Set of unique observer objects.
Copy link
Member

Choose a reason for hiding this comment

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

are these objects that observe, or objects that are being observed?

what makes something an "observer"?

is there a base class somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated the observer class in PR #181 maybe I remove from here, so is less confusing

@Licini Licini merged commit b3b0118 into main Jun 26, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants