jupytercad / JupyterCAD

A JupyterLab extension for 3D geometry modeling
https://jupytercad.github.io/JupyterCAD
BSD 3-Clause "New" or "Revised" License
135 stars 18 forks source link

Add Color Customization for JupyterCAD Models #397

Closed arjxn-py closed 2 months ago

arjxn-py commented 2 months ago

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

github-actions[bot] commented 2 months ago

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

trungleduc commented 2 months ago

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 commented 2 months ago

@trungleduc I suggested getting rid of guidata in here: https://github.com/jupytercad/JupyterCAD/issues/394

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

trungleduc commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

https://github.com/user-attachments/assets/6843d942-c531-49ef-bffc-94c380b01cd6

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

https://github.com/user-attachments/assets/fb52ba73-e185-4791-826f-45f0e3530b08

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

martinRenou commented 2 months ago

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 commented 2 months ago

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.

Thanks @martinRenou, I'll do it and follow up :)

arjxn-py commented 2 months ago

https://github.com/user-attachments/assets/26424d86-cd4a-434a-80aa-cbc943dff012

Works even better with color picker that I myself am impressed 🤣 Thanks @martinRenou

arjxn-py commented 2 months ago

Hi @martinRenou, I think it'd be great to have your input here about which other schemas I should add Color property to?

arjxn-py commented 2 months ago

Thanks for the comprehensive review! I'll be on it to add Color property to all the remaining required schemas. And I plan to handle these operations one after another iteratively hence first one in my list would be chamfer, if that sounds alright :)

arjxn-py commented 2 months ago

Just wanted to confirm that if I don't need to add Color property to geomCircle, geomLineSegment etc. (i.e. Skip those which do not have placement prop?) If i should, I can quickly add too.

martinRenou commented 2 months ago

Skip those which do not have placement prop

Yes that sounds correct 👍🏽

arjxn-py commented 2 months ago

The fuse operator may be trickier, should we blend the colors to get a new one? Happy to get suggestions on this.

I do not have much idea about fuse operator at the moment to have an opinion. But I shall workaround it and then have a valid suggestion for the same. In the meantime, it should be great to have @trungleduc's suggestion on it too. Thanks :)

trungleduc commented 2 months ago

In JupyterCAD, the boolean operator output is a new shape, I would use the new shape default color

martinRenou commented 2 months ago

In JupyterCAD, the boolean operator output is a new shape, I would use the new shape default color

It would look so much nicer to keep the properties of the original object though, from a user point of view

martinRenou commented 2 months ago

I was actually thinking that doing that for the placement could be nice too

martinRenou commented 2 months ago

That's what FreeCAD does:

https://github.com/user-attachments/assets/12e14208-35b5-4d69-bcf7-7e3a6a8c511e

arjxn-py commented 2 months ago

In JupyterCAD, the boolean operator output is a new shape, I would use the new shape default color

It would look so much nicer to keep the properties of the original object though, from a user point of view

Yes, it could be tried to preserve the properties of the object after operation if not already done. As far as I anticipate, Once done for color, Same could be implemented for placement too.

martinRenou commented 2 months ago

At least, I was thinking to automatically pre-fill the creation form with the Base's color value for the operators that have a Base.

The user can always change the color in the form.

arjxn-py commented 2 months ago

That's what FreeCAD does:

looks like they're not blending but the cylinder's surface in contact to the cube has the color of cube. (could be trickier but first step should be to preserve the cylinder's color for us in the same operation in JCAD)

martinRenou commented 2 months ago

surface in contact to the cube has the color of cube. (could be trickier but first step should be to preserve the cylinder's color for us in the same operation in JCAD)

Ah yes indeed I missed that!

Unfortunately we don't have any logic to render mesh faces separately

arjxn-py commented 2 months ago

Unfortunately we don't have any logic to render mesh faces separately

Yes, this could be a long term goal as it might need a good amount of refactoring too.

Though I saw these (might not be totally related to our case but somewhat similar) -

martinRenou commented 2 months ago

Yes that would be way too much refactoring. I'm not even sure we'd reach that point ever in JupyterCAD.

arjxn-py commented 2 months ago

Yes that would be way too much refactoring. I'm not even sure we'd reach that point ever in JupyterCAD.

Yeah, so I think just opening an issue to keep track of that should be enough for now and keeping the mesh color as before should work for the time being?

trungleduc commented 2 months ago

I was actually thinking that doing that for the placement could be nice too

Yes, it could be tried to preserve the properties of the object after operation if not already done. As far as I anticipate, Once done for color, Same could be implemented for placement too.

No, placement should default to the zeros unless users modify it. The output of the boolean operator has already taken into account the placements of input objects.

The placement is used to translate and rotate the local axes of an object in the global axes after its construction.

martinRenou commented 2 months ago

placement should default to the zeros unless users modify it. The output of the boolean operator has already taken into account the placements of input objects.

Right 👍🏽

arjxn-py commented 2 months ago

Cut:

https://github.com/user-attachments/assets/de7bd57a-eba5-4d3d-88aa-13e5049ac8bc

Union:

https://github.com/user-attachments/assets/566e3815-72b7-4c87-9047-13b47c3a3f93

Intersection:

https://github.com/user-attachments/assets/20b0652f-c986-495f-b8a9-2f4f3dca58e7

arjxn-py commented 2 months ago

I've given baseModel's color after each operation, feel free to let me know if that's not user friendly as I anticipated that it is. These colors can always be updated.

arjxn-py commented 2 months ago

Bot please update snapshots!

arjxn-py commented 2 months ago

Oh! Oops, looks like bot is working(I thought it was not as it took much time) But I think #400 should still be relevant, if not feel free to close :)

arjxn-py commented 2 months ago

Bot please update snapshots!

arjxn-py commented 2 months ago

Bot please update snapshots!

Looks like bot is angry again 😭

martinRenou commented 2 months ago

Ah right, the bot can only run if the build workflow passed already, because it reuses the build artifacts instead of making a custom build. I'll restart it now, that should be enough.

martinRenou commented 2 months ago

Damned, it still doesn't want. Bot please update snapshots!!!

martinRenou commented 2 months ago

Ah I know what's wrong. We need this https://github.com/geojupyter/jupytergis/blob/main/.github/workflows/update_galata_references.yaml#L79

arjxn-py commented 2 months ago

Ah I know what's wrong. We need this https://github.com/geojupyter/jupytergis/blob/main/.github/workflows/update_galata_references.yaml#L79

I see, Should I send a PR?

martinRenou commented 2 months ago

Please do! It cannot be done in this PR because the bot runs from main, so we need to update main to apply

arjxn-py commented 2 months ago

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

martinRenou commented 2 months ago

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 commented 2 months ago

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

arjxn-py commented 2 months ago

Bot please update snapshots! :face_with_thermometer:

arjxn-py commented 2 months ago

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 commented 2 months ago

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

arjxn-py commented 2 months ago

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.