Skip to content

Conversation

@romanarust
Copy link
Member

This PR adds several features and suggests some little modifications. Fixes #335.
Checkout Changelog for a complete list of additions and changes.

Comes with 2 incompatible API changes:

  • To streamline with the rest, changed parameters origin, uvw of global_coords_numpy and local_coords_numpy to frame
  • Renamed Frame.represent_point/vector/frame_in_global_coordinates and Frame.represent_point/vector/frame_in_local_coordinates to Frame.local_coords and Frame.global_coords

What type of change is this?

  • Bug fix in a backwards-compatible manner.
  • New feature in a backwards-compatible manner.
  • Breaking change: bug fix or new feature that involve incompatible API changes.

Checklist

  1. Add the change to the CHANGELOG.md file in the Unreleased section under the most fitting heading: Added, Changed, Removed.
  2. Run all tests on your computer (i.e. invoke test).
  3. If you add new functions/classes, check that:
    1. Are available on a second-level import, e.g. compas.datastructures.Mesh.
    2. Add unit tests (especially important for algorithm implementations).

Copy link
Member

@tomvanmele tomvanmele left a comment

Choose a reason for hiding this comment

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

nice! we just need to work on the naming conventions here and there in my opinion...


### Added

- Added `matrix_change_basis`, `Transformation.change_basis`
Copy link
Member

Choose a reason for hiding this comment

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

should this not be matrix_from_basis_change or something like that?

Copy link
Member Author

@romanarust romanarust Oct 15, 2019

Choose a reason for hiding this comment

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

better would be change_of_basis_matrix but that does not follow the other naming.
or matrix_to_change_basis?

Copy link
Member

Choose a reason for hiding this comment

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

ha no, sorry, i misunderstood the point of the function...

"""Represents another frame in the local coordinate system in the world
coordinate system.
@staticmethod
def local_to_local_coords(frame1, frame2, object_in_frame1):
Copy link
Member

Choose a reason for hiding this comment

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

same as with global (and also applies everywhere) should local not be frame?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree to the global to world. But with I am not sure with the frame to local.
While it works well with the function-based version (world_to_frame_coords(frame, point) or world_to_frame(frame, point)), the oo-version does not look so nice: frame.to_frame(point) or frame.to_frame_coords(point). There I would prefer keeping the frame.local_coords(point).

Some options:

oo function
frame.local_coords(point), frame.world_coords(point), frame.to_local(point), frame.to_world(point), world_to_local(frame, point), world_to_local_coords(frame, point), local_to_world_coords(frame, point), local_to_world(frame, point)
frame.to_frame(point), frame.to_world(point), frame.to_frame_coords(point), frame.to_world_coords(point) world_to_frame(frame, point), world_to_frame_coords(frame, point), frame_to_world_coords(frame, point), frame_to_world(frame, point)
Frame.local_to_local_coords(f1, f2, point), Frame.frame_to_frame_coords(f1, f2, point) local_to_local_coords(f1, f2, point), frame_to_frame_coords(f1, f2, point)

more opinons, ideas, .. are welcome ;)

Copy link
Member

Choose a reason for hiding this comment

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

I prefer local much more than frame (frame is very ambiguous imo).
I like the option with to_ prefixes.

Copy link
Member

Choose a reason for hiding this comment

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

alright, local it is.
to_ versions +1

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, thank you! implemented changes

return multiply_matrices(T2, matrix_inverse(T1))


def matrix_change_basis(frame_from, frame_to):
Copy link
Member

Choose a reason for hiding this comment

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

how is this different than frame_to_frame?

Copy link
Member Author

Choose a reason for hiding this comment

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

the frame_to_frame transformation from frame f1 to frame f2 allows to transform geometry from f1 into f2, i.e. the orientation of the object in regards to the respective frame stays the same.
The change_basis transformation from frame f1 to frame f2 allows to represent geometry of frame f1 in frame f2 (not transforming the geometry )
so they are used for different purposes...

@tomvanmele
Copy link
Member

@gonzalocasas merge?

@tomvanmele tomvanmele merged commit 6ec660b into master Oct 18, 2019
@tomvanmele tomvanmele deleted the update-frame-and-transformation branch August 6, 2020 09:56
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.

Transform multiple frames

4 participants