Placement.rotate(center, axis, angle): wrong handling of center?
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Be nice to others! Respect the FreeCAD code of conduct!
Re: Placement.rotate(center, axis, angle): wrong handling of center?
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
The current implementation has certainly confounded the Draft programmers:
https://github.com/FreeCAD/FreeCAD/pull/7267/files
-
- Veteran
- Posts: 3106
- Joined: Thu Sep 24, 2020 10:31 pm
- Location: Hawaii
- Contact:
Re: Placement.rotate(center, axis, angle): wrong handling of center?
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.
Re: Placement.rotate(center, axis, angle): wrong handling of center?
"Comp" as in Compatible with TopoShape.rotate().
-
- Veteran
- Posts: 3106
- Joined: Thu Sep 24, 2020 10:31 pm
- Location: Hawaii
- Contact:
Re: Placement.rotate(center, axis, angle): wrong handling of center?
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.
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()
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
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)
Re: Placement.rotate(center, axis, angle): wrong handling of center?
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.
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.
Re: Placement.rotate(center, axis, angle): wrong handling of center?
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)
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)
Re: Placement.rotate(center, axis, angle): wrong handling of center?
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:
Appears to work with the initial script provided. I'm going to prepare a pull request.
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).
Last edited by mfro on Fri Jul 29, 2022 11:04 am, edited 1 time in total.
Cheers,
Markus
Markus
Re: Placement.rotate(center, axis, angle): wrong handling of center?
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
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