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

Add Color Customization for JupyterCAD Models #397

Merged
merged 32 commits into from
Sep 9, 2024

Conversation

arjxn-py
Copy link
Member

@arjxn-py arjxn-py commented Sep 2, 2024

This PR adds a new property "Color" in JupyterCAD objects, that allows users to change the color of individual objects within a model.
image

related to #394

@arjxn-py arjxn-py marked this pull request as draft September 2, 2024 06:14
Copy link
Contributor

github-actions bot commented Sep 2, 2024

Binder 👈 Launch a Binder on branch arjxn-py/JupyterCAD/custom-color

@trungleduc
Copy link
Member

Thank you for working on it. Adding a new Color property would make it harder to maintain compatibility with the FreeCAD file format. Why don't we reuse the guidata logic?

@martinRenou
Copy link
Member

@trungleduc I suggested getting rid of guidata in here: #394

I believe we should do the guidata -> color/transparency translation in Python in our freecad handler. What do you think?

@trungleduc
Copy link
Member

trungleduc commented Sep 2, 2024

@trungleduc I suggested getting rid of guidata in here: #394

I believe we should do the guidata -> color/transparency translation in Python in our freecad handler. What do you think?

👍, fcstd -> jcad should be easy, I'm not sure about jcad -> fcstd

@martinRenou
Copy link
Member

I'm not sure about jcad -> fcstd

It should also be fairly easy, it is just a dictionary {obj_name: options} with options being {ShapeColor: color, 'Visibility': visible} https://github.com/jupytercad/JupyterCAD-FreeCAD/blob/main/jupytercad_freecad/freecad/loader.py#L48

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 3, 2024

jcad-color.mp4

Setting custom color here in mainview changes the color of the object but on first interaction it comes back to it's original color.
I'm also a little unsure about how do i access the color property from object schema in mainview? I had been trying on that yesterday but I think it'd be very much helpful to have your suggestion for that. Thanks :)

More context: I've been logging and observing guidata to see if it has the object metadata and hence the color property. But could only find :

{Box 1: {…}}Box 1: visibility: true
[[Prototype]]: Object

@martinRenou
Copy link
Member

martinRenou commented Sep 3, 2024

You would also need to set the color here when building the shapes. This gets called whenever there is a change in the object (size, position etc).

You should be able to get the color property around here:
https://github.com/jupytercad/JupyterCAD/blob/main/packages/base/src/3dview/mainview.tsx#L593
with something like obj.parameters.Color, then pass it to buildShape.

Note that you only changed the schema for the box, you'd need to change all schemas that should define a color.

@martinRenou
Copy link
Member

More context: I've been logging and observing guidata to see if it has the object metadata and hence the color property. But could only find :

We'll eventually get rid of guidata, maybe in this PR, so you should not depend on it.

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 3, 2024

Thanks a lot, this really helps 💐

Note that you only changed the schema for the box, you'd need to change all schemas that should define a color.

Yes, I'm planning to do that once I have a proof of concept for box ready if that sounds fine. I can do it now as well if you'd like :)

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 3, 2024

More context: I've been logging and observing guidata to see if it has the object metadata and hence the color property. But could only find :

We'll eventually get rid of guidata, maybe in this PR, so you should not depend on it.

Makes sense, I'll try to avoid using it, thanks for validating.

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 4, 2024

jcad-color.1.mp4

Works nicely with box for me 🎉
now on the other shapes

@martinRenou
Copy link
Member

Looks great! I have a small suggestion, you should be able to get a nice and shiny color picker instead of a text input if you change the uiSchema for the form building logic.

Something like:

uiSchema['Color'] = {
  'ui:widget': 'color'
};

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 6, 2024

Should we update branch with respect to main if it's necessary or we can straight up request bot without waiting for that?

@martinRenou
Copy link
Member

martinRenou commented Sep 6, 2024

It seems the bot did pick up the change https://github.com/jupytercad/JupyterCAD/actions/runs/10740182227/job/29787656302 (I see Allow forks: true) but it's still not happy. Let's wait for the CI to finish and restart the bot.

@martinRenou
Copy link
Member

Yep https://github.com/jupytercad/JupyterCAD/actions/runs/10740182227/job/29787926016 CI needs to have finished before the bot can run

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 6, 2024

Bot please update snapshots! 🤒

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 6, 2024

Yep https://github.com/jupytercad/JupyterCAD/actions/runs/10740182227/job/29787926016 CI needs to have finished before the bot can run

Ahh! I missed this comment. Sorry
Hope it doesn't mess it up more

@martinRenou
Copy link
Member

It's fine, the second run will just cancel eventually

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 6, 2024

Updating branch w.r.t. main should trigger CI fully, should i do it now?

Edit: Doing it now to save some time, hope it's alright.

@arjxn-py arjxn-py requested a review from martinRenou September 7, 2024 09:12
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks! I have a couple more comments.

packages/base/src/3dview/mainview.tsx Outdated Show resolved Hide resolved
packages/base/src/3dview/mainview.tsx Show resolved Hide resolved
@arjxn-py arjxn-py requested a review from martinRenou September 9, 2024 10:19
@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 9, 2024

image
Edges are no longer black or DEFAULT_EDGE_COLOR.
Honestly, it personally looks good to me. What do you think?

@martinRenou
Copy link
Member

Honestly, it personally looks good to me. What do you think?

I'm not sure about this change. This makes edges less visible, but those are clickable so they should be easily spotted by the user.

@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 9, 2024

I'm not sure about this change. This makes edges less visible, but those are clickable so they should be easily spotted by the user.

Which one do you prefer though?

@martinRenou
Copy link
Member

Visually I prefer them in your screenshot, from the UX point of view I prefer them black.

Though once the object is black it won't be visible anymore.

We may want a smart way to compute the edge color depending on the mesh color so that it's highlighted enough for us to see them.

@martinRenou
Copy link
Member

Let's not do that in this PR though

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 🚀

As a follow-up:

  • get rid of guidata usage in the codebase (we will eventually need to introduce an "Visible" property to the objects)
  • (Optional) figure out a smart way to compute the edge colors so that they are still visible but more pretty (make use of d3-color to make the color darker/brighter https://d3js.org/d3-color#color_darker)

@martinRenou martinRenou merged commit a15214a into jupytercad:main Sep 9, 2024
9 checks passed
@arjxn-py
Copy link
Member Author

arjxn-py commented Sep 9, 2024

  • get rid of guidata usage in the codebase (we will eventually need to introduce an "Visible" property to the objects)
  • (Optional) figure out a smart way to compute the edge colors so that they are still visible but more pretty (make use of d3-color to make the color darker/brighter https://d3js.org/d3-color#color_darker)

Thanks a lot @martinRenou, I'll be on these :)

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