search

FreeCAD / FreeCAD-macros

A repository for FreeCAD macros
https://freecad.org/wiki/Macros_recipes
145 stars 134 forks source link

[CenterOfMass]: Add bounding box #128

Closed chupins closed 2 years ago

chupins commented 2 years ago

Draw a bounding box around all selected object when you choose to look at the center of masses

chupins commented 2 years ago

Little changes in the layout to improve view options for the equivalent spheres (possibility to switch from mass comparison to volume comparison of selected objects)

s-quirin commented 2 years ago

@chupins We better should test it internally first and not confuse the user with numerous updates. Please check out my fork with Bugfixes.

chupins commented 2 years ago

@s-quirin, my modifications were purely "comestic" and doesn't need any internal testing. But I agree we have to find a way to work on the same file.

By now, I push my changes on my fork and then I ask for merge here. I not extremely familiar with git, then I don't know if there is a better way to proceed. Is there a way to pull from your fork and mine ?

chupins commented 2 years ago

@s-quirin, I merged your changes in my fork. And then, it's in the present merge request

s-quirin commented 2 years ago

@s-quirin, I merged your changes in my fork. And then, it's in the present merge request

Not correctly, see above, broken stuff in code. Also broken: colorify shapes, e.g. directly after starting up the macro. Recommend #129 first and after that add your new logic.

chupins commented 2 years ago

Sorry but I can't see anything above that clearly explains what's broken ! I don't know if it's the proper way to do it, but your MR contains a lot of thing that are already pending here.

chupins commented 2 years ago

A lot of changes made by s-quirin in #129 were already made here. So I manually merged its version in here and took @galou remarks into account.

@s-quirin can you check if you find any problem ? When I do a diff between your version and this one, I can't find any fundamental difference (the only things are my new colormaps and the mass/volume button)

galou commented 2 years ago

@chupins: @s-quirin removed some files in his PR, can you do the same if these files are indeed not needed.

I'll wait for @s-quirin's approval before merging.

galou commented 2 years ago

@chupins: @s-quirin removed some files in his PR, can you do the same if these files are indeed not needed.

I'll wait for @s-quirin's approval before merging.

chupins commented 2 years ago

You're right again, I forgot to delete these unecessary files. We'll see with @s-quirin how we merge this PR and the #129.

galou commented 2 years ago

@chupins @s-quirin I don't know whether I should merge this PR, #129, or both. Can you please express yourself here? Thanks!

s-quirin commented 2 years ago

@chupins @s-quirin I don't know whether I should merge this PR, #129, or both. Can you please express yourself here? Thanks!

We couldn't come to an agreement yet because of holidays / lack of time. #128 mainly contains two controversial changes that emerged in my own test and with end users:

I asked @chupins to fix them in this PR or to withdraw it and go with #129.

chupins commented 2 years ago

Sorry for the delay, as @s-quirin said, lack of time and hollidays aren't helping !

If it's ok for you @galou, @s-quirin and me are converging together on a version and at the end, we'll close this PR (#128) and the "official PR" will be the #129.

I just find something bugging on #129 but we're close to finish it. (then, we'll look at #130 too :))

chupins commented 2 years ago

@s-quirin has added everything on #129 . We prefer canceling here and ask if #129 can be accepted.