Placement.rotate(center, axis, angle): wrong handling of center?

Need help, or want to share a macro? Post here!
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
Roy_043
Veteran
Posts: 8450
Joined: Thu Dec 27, 2018 12:28 pm

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by Roy_043 »

Instead of a keyword argument you could add a new method: Placement.rotateComp(center, axis, angle).

The current implementation has certainly confounded the Draft programmers:
https://github.com/FreeCAD/FreeCAD/pull/7267/files
edwilliams16
Veteran
Posts: 3106
Joined: Thu Sep 24, 2020 10:31 pm
Location: Hawaii
Contact:

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by edwilliams16 »

Roy_043 wrote: Thu Jul 28, 2022 3:50 pm Instead of a keyword argument you could add a new method: Placement.rotateComp(center, axis, angle).
IMO, that looks like a good route to take. Both methods have valid uses, so provide both. The name should be some rotateXXX(), but rotateComp() isn't stirring any image of what it does. What did you have in mind?
Writing the help strings without writing an essay could be a challenge. They definitely should refer to each other as performing the placements in the opposite order, so that the pragmatic user can at least just try the other one if s/he gets unexpected results.
User avatar
Roy_043
Veteran
Posts: 8450
Joined: Thu Dec 27, 2018 12:28 pm

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by Roy_043 »

"Comp" as in Compatible with TopoShape.rotate().
edwilliams16
Veteran
Posts: 3106
Joined: Thu Sep 24, 2020 10:31 pm
Location: Hawaii
Contact:

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by edwilliams16 »

Roy_043 wrote: Thu Jul 28, 2022 5:33 pm "Comp" as in Compatible with TopoShape.rotate().
OK. Current help:

Code: Select all

Help on built-in function rotate:

rotate(...) method of Base.Placement instance
    rotate(center, axis, angle) -> None
    
    Rotate the current placement around center and axis with the given angle.
    This method is compatible with TopoShape.rotate().
    
    center : Base.Vector, sequence of float
        Rotation center.
    axis : Base.Vector, sequence of float
        Rotation axis.
    angle : float
        Rotation angle in degrees.
Suggestion:

Code: Select all

Help on built-in function rotate:

rotate(...) method of Base.Placement instance
    rotate(center, axis, angle) -> None
    
    Rotate the current placement around center and axis with the given angle.
    
    center : Base.Vector, sequence of float
        Rotation center.
    axis : Base.Vector, sequence of float
        Rotation axis.
    angle : float
        Rotation angle in degrees.
        
    In this method, the current placement is applied after the rotation.
    In the related method rotateComp(), the current placement is applied before the rotation - as in TopoShape.rotate()
For rotateComp()

Code: Select all

Help on built-in function rotateComp:

rotateComp(...) method of Base.Placement instance
    rotateComp(center, axis, angle) -> None
    
    Rotate the current placement around center and axis with the given angle.
    
    center : Base.Vector, sequence of float
        Rotation center.
    axis : Base.Vector, sequence of float
        Rotation axis.
    angle : float
        Rotation angle in degrees.
        
    In this method, the current placement is applied before the rotation - as in TopoShape.rotate()
    In the related method rotate(), the current placement is applied after the rotation 
or is that too pithy?
EDIT
i.e Do we want to say explicitly that
current.rotate(center, axis angle) changes current to current.multiply(App.Placement(App.Vector(0, 0, 0), App.Rotation(axis, angle), center)) and
currentComp.rotate(center, axis angle) changes current to App.Placement(App.Vector(0, 0, 0), App.Rotation(axis, angle), center).multiply(current)
User avatar
Roy_043
Veteran
Posts: 8450
Joined: Thu Dec 27, 2018 12:28 pm

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by Roy_043 »

edwilliams16 wrote: Thu Jul 28, 2022 5:55 pm Do we want to say explicitly that...
Why not? I would prefer to see all important information above the specification of the arguments. And the summary of rotate should be different IMO.

New Help for rotate:

Code: Select all

Help on built-in function rotate:

rotate(...) method of Base.Placement instance
    rotate(center, axis, angle) -> None

    Rotate a default placement around center and axis with the given angle, and multiply the current placement with the result.

    This is equivalent to:
    current = current.multiply(App.Placement(App.Vector(0, 0, 0), App.Rotation(axis, angle), center))

    This method is not compatible with TopoShape.rotate(), the related rotateComp() method is.

    center : Base.Vector, sequence of float
        Rotation center.
    axis : Base.Vector, sequence of float
        Rotation axis.
    angle : float
        Rotation angle in degrees.
Help for rotateComp:

Code: Select all

Help on built-in function rotateComp:

rotateComp(...) method of Base.Placement instance
    rotateComp(center, axis, angle) -> None

    Rotate the current placement around center and axis with the given angle.

    This is equivalent to:
    current = App.Placement(App.Vector(0, 0, 0), App.Rotation(axis, angle), center).multiply(current)

    This method is compatible with TopoShape.rotate().

    center : Base.Vector, sequence of float
        Rotation center.
    axis : Base.Vector, sequence of float
        Rotation axis.
    angle : float
        Rotation angle in degrees.
heda
Veteran
Posts: 1348
Joined: Sat Dec 12, 2015 5:49 pm

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by heda »

hm, is it really that hard to make it a keyword?
not saying that i have a clue of to do it, or understand what is involved, but hoping there are people around that could do it without too much hazzle

why, well a keyword is imho a far better choice for bigger picture in terms of api
it makes the user directly aware of that there are 2 ways, and one can just try the 2 ways
when it is two different function names, the cognitive load to do the same is increased by multiples

ot: imho the "1.0" discussion is lacking the aspect of "wart-ironing", do of course realize that tnp is all the rage for 1.0, just miss the discussion on other things that makes people go "what/why" and if there is potential to smoothen that experience (not talking ui here, but rather api/inner workings/sw architecture)
User avatar
mfro
Posts: 663
Joined: Sat Sep 23, 2017 8:15 am

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by mfro »

I apologise. Looked into the keyword argument thing again and found that it is a lot easier to implement than I initially thought.
Actually, it's done already. This is what I just implemented and right now have under test:

Code: Select all

>>> help(App.Placement.rotate)
Help on method_descriptor:

rotate(...)
    rotate(center, axis, angle, kwargs) -> None
    
    Rotate the current placement around center and axis with the given angle.
    This method is compatible with TopoShape.rotate() if the (optional) keyword
    argument comp is True (default=False).
    
    
    center : Base.Vector, sequence of float
        Rotation center.
    axis : Base.Vector, sequence of float
        Rotation axis.
    angle : float
        Rotation angle in degrees.
    comp : bool
        optional keyword only argument, if True (default=False),
        behave like TopoShape.rotate() (i.e. the resulting Placements are interchangable).
Appears to work with the initial script provided. I'm going to prepare a pull request.
Last edited by mfro on Fri Jul 29, 2022 11:04 am, edited 1 time in total.
Cheers,
Markus
heda
Veteran
Posts: 1348
Joined: Sat Dec 12, 2015 5:49 pm

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by heda »

cool :!:
llll
Posts: 173
Joined: Fri Nov 12, 2021 1:56 am

Re: Placement.rotate(center, axis, angle): wrong handling of center?

Post by llll »

I am still unable to pivot about a point without the influence of CentreofGravity.

How do I turn off CentreofGravity so I can set the real center of rotation matrix?
Has me crawling up the walls

I assume its caused by CoG since the pivot about point works on simpler objects
Post Reply