Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default orientation Bug #60

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

philipp1604
Copy link
Collaborator

@philipp1604 philipp1604 commented Sep 5, 2024

Description

This PR updates both the linear_interpolation and circular_interpolation functions to handle quaternion interpolation using Spherical Linear Interpolation (SLERP) through RotationSpline. Additionally, the functions now correctly handle cases where end_orientation is not provided. When end_orientation is None, the functions default to using start_orientation for the entire path, ensuring consistent behavior.

The test suite has been updated to include combined test cases for both linear and circular interpolation. These tests validate scenarios with both start_orientation and end_orientation, as well as the case where only start_orientation is provided.

Additionally, an error in circular path generation has been fixed by adjusting the angle step calculation. Dividing by step_num - 1 now ensures accurate angular range coverage. It ensures that the start and end points align precisely with the input angles.

The GCodeProcessor class was updated due to a misconception in sampling size, as it always corresponds to the total number of samples in a path including start and end point. Two calculate the correct interpolation step precision + 2 has to be added to the interpolation_steps.

Motivation and Context

Previously, quaternion interpolation relied on methods that didn't correctly handle SLERP, which could result in unexpected behavior, particularly in cases involving smooth rotational transitions. By using RotationSpline, this PR ensures that quaternion interpolation is smooth and handles edge cases like gimbal lock more effectively.

In cases where end_orientation is omitted, the functions now default to start_orientation, making it easier for users who expect the object to maintain a consistent orientation along the path. This update enhances the robustness and usability of both linear_interpolation and circular_interpolation.

How has this been tested?

  • The test suite was expanded and refactored to include both cases (with and without end_orientation) for both linear_interpolation and circular_interpolation.
  • The tests use RotationSpline to generate expected SLERP results, ensuring consistency between the actual and expected behavior.

Screenshots (if appropriate):

N/A

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

- If end_orientation is not provided, it is now correctly defaulted to start_orientation, ensuring the object maintains a consistent orientation throughout the interpolated path.
@philipp1604
Copy link
Collaborator Author

@liquidcronos @MaHajo
This is the Pull-Request for Issue #58 . Before merging we need to close Issue #59 first.

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.32%. Comparing base (9cf88ec) to head (4e354a6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
+ Coverage   88.14%   88.32%   +0.18%     
==========================================
  Files          17       17              
  Lines        1130     1131       +1     
==========================================
+ Hits          996      999       +3     
+ Misses        134      132       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This change was necessary in order to pass the code coverage test for the update of the interpolation-orientation branch
this change was required in order to pass the test for code coverage
@philipp1604
Copy link
Collaborator Author

Hello everyone @liquidcronos @MaHajo ,
this branch should be ready for merging. In addition to solving the bug, I had to add a suitable interpolation call to the test_grippers class to pass the code coverage test. This change did not affect the behavior of the test_grippers class.

philipp1604 and others added 8 commits October 24, 2024 11:24
- This forms a test class for the interpolation method
- the spline interpolation is left out
- this is the first approach of spherical interpolating between orientations
- combining test cases for each interpolation
Fixed angle step calculation in circular path generation by dividing by step_num - 1 to ensure proper angular range coverage and avoid over/undersampling
interpreting the sample size correctly
- changing sampling to number of points between start and end
- sampling size should always be at least 2, as start and end point always have to be part of the samples
- some classes like test_endeffector use the sampling size wrong, but without changing there test methods, this bug can not be fixed
- this branch is ready for merge
rotations = R.from_quat([start_rot.as_quat(), end_rot.as_quat()])
t_vals = np.linspace(0, 1, samples)
slerp_spline = RotationSpline([0, 1], rotations)
expected_orientations_both = slerp_spline(t_vals).as_quat()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to have preset expected orientations without using the RotationSpline function? (analog to expected_positions)

rotations = R.from_quat([start_rot.as_quat(), end_rot.as_quat()])
t_vals = np.linspace(0, 1, samples)
slerp_spline = RotationSpline([0, 1], rotations)
expected_orientations_both = slerp_spline(t_vals).as_quat()
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

result_with_only_start_orientation.orientations,
expected_toolpath_with_start_only.orientations, decimal=6)

def test_zero_sample_size(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

missing function description

Update GitHub Actions workflows to specify Ubuntu version to ensure compatibility and avoid issues with the upcoming change of ubuntu-latest to ubuntu-24.04."

actions/runner-images#10636
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@MaHajo Update GitHub Actions workflows to specify Ubuntu version to ensure compatibility and avoid issues with the upcoming change of ubuntu-latest to ubuntu-24.04." After January 17th, 2025 we should be able to change back to ubuntu-latest

actions/runner-images#10636

Copy link
Collaborator Author

@philipp1604 philipp1604 Dec 13, 2024

Choose a reason for hiding this comment

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

@MaHajo with the new ubuntu 24.04 version this change was necessary.

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.

2 participants