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

Fix/obj setting #181

Merged
merged 29 commits into from
Jul 18, 2024
Merged

Fix/obj setting #181

merged 29 commits into from
Jul 18, 2024

Conversation

PingHsunTsai
Copy link
Collaborator

@PingHsunTsai PingHsunTsai commented Jun 20, 2024

What type of change is this?

#183

  • Added DefaultLayout to handle gerneral layout setting to minimal.
  • Added TextEdit to handle name change.
  • Added Observer to handle is_selected state update or more.

@PingHsunTsai PingHsunTsai self-assigned this Jun 20, 2024
@PingHsunTsai
Copy link
Collaborator Author

The modified obj attributes are able to save to scene, but there is a problem to load the color attribute from scene, we can fo it in another PR

@Licini
Copy link
Collaborator

Licini commented Jun 26, 2024

This PR is now mixed with fixing the layout of object settings, and implementation of Observer which we might need more iterations vs Signal pattern. Can you isolate out the Observer part?

@PingHsunTsai
Copy link
Collaborator Author

Yes, but the ability to open ObjectSetting from Sceneform selection will not be available for now(just a heads up).
I will try is there another quick solution.

@Licini
Copy link
Collaborator

Licini commented Jun 26, 2024

How about add the object settings cmd as callback function in the config of SceneForm?

{
    "type": "Sceneform",
    "columns": [
        {"title": "Name", "type": "label", "text": lambda obj: obj.name},
        {"title": "Show", "type": "checkbox", "checked": lambda obj: obj.show, "action": lambda obj, checked: setattr(obj, "show", checked)},
    ],
    "on_select": obj_settings_cmd # something like this
},

@PingHsunTsai
Copy link
Collaborator Author

yes on it!

@Licini
Copy link
Collaborator

Licini commented Jun 26, 2024

How about add the object settings cmd as callback function in the config of SceneForm?

{
    "type": "Sceneform",
    "columns": [
        {"title": "Name", "type": "label", "text": lambda obj: obj.name},
        {"title": "Show", "type": "checkbox", "checked": lambda obj: obj.show, "action": lambda obj, checked: setattr(obj, "show", checked)},
    ],
    "on_select": obj_settings_cmd # something like this
},

Just realized there was already a callback parameter in SceneForm which does exactly this, you can use that directly. Let's just use name callback instead of on_select

@Licini
Copy link
Collaborator

Licini commented Jun 26, 2024

Some observations here:

  1. the object setting form should hide when nothing selected
image
  1. Name should not be number
image
  1. Color picker is too thin in vertical direction
image
  1. The object form should be in a scrollable, right now the form has a fixed size, the portion between scene form and object form should be adjustable

@PingHsunTsai
Copy link
Collaborator Author

regarding your first point @Licini
I think Tom prefer to have a placeholder there, but I just added self.hide_widget = True option in SideBarRight, so this can be decided by user as well.
For the scrolling space, it needs to be restructure a bit, cus it takes different type than layout, so I will propose we do it in another PR
The rest I will try to adjust them now.

@PingHsunTsai
Copy link
Collaborator Author

Hi @Licini and @tomvanmele this PR is ready!

@Licini
Copy link
Collaborator

Licini commented Jul 12, 2024

Noticed some issues with spinbox:

  1. Plz make the spinbox wider and show the digits after
  2. use 1 decimal place instead of two
  3. Make the step 0.1 instead of 1, right now if I click down arrow it set to value 0 and start to throw render errors. (linewidth value can not <= 0) .
image

Copy link
Collaborator

@Licini Licini left a comment

Choose a reason for hiding this comment

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

Looking much better! but still plz make the changes mentioned above

@Licini
Copy link
Collaborator

Licini commented Jul 12, 2024

one more question, how do you disable the object settings?

@PingHsunTsai
Copy link
Collaborator Author

@Licini d9369e2
I just add a setter here

@PingHsunTsai PingHsunTsai requested a review from Licini July 12, 2024 15:43
@Licini
Copy link
Collaborator

Licini commented Jul 18, 2024

Thanks @PingHsunTsai ! Merging Finally!

@Licini Licini merged commit b942be5 into main Jul 18, 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.

3 participants