Skip to content

Feature: reset_camera - #172

Open
olive-tree-branch wants to merge 3 commits into
thoth-tech:mainfrom
olive-tree-branch:Feature/reset_camera
Open

Feature: reset_camera#172
olive-tree-branch wants to merge 3 commits into
thoth-tech:mainfrom
olive-tree-branch:Feature/reset_camera

Conversation

@olive-tree-branch

Copy link
Copy Markdown

Description

This function is a simple on that I had thought of after fixing PR #168 where there was not a clean way to reset the camera back to (x = 0, y = 0).

The purpose of this function will make it nicer for users to write code that needs to reset the camera and helps with readability.

I have updated the integration test with this PR to use the new function and I made this a separate PR to #168 as that was a bugfix and this a new feature.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation (update or new)

How Has This Been Tested?

This has been tested in the camera integration test that was originally having problems with cleanup (#168) and generates no new errors.

Testing Checklist

  • Tested with sktest

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have requested a review on the Pull Request

@jankiluitel jankiluitel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for adding reset_camera. The implementation is clear and appropriately resets both camera coordinates to zero, and the API documentation explains the intended behaviour well.

I have two suggestions before approval:

The current test calls reset_camera(), but it does not appear to verify the resulting camera position. Could you please add an assertion/check confirming that the camera X and Y positions are both 0 after calling reset_camera()? Ideally, the test should first move the camera away from (0, 0), call reset_camera(), and then verify that it has returned to the expected position.
drawing_options.cpp appears to contain whitespace-only changes unrelated to the reset_camera feature. It would be good to revert those changes so the PR remains focused on the camera functionality.

Once these are addressed, the change should be much easier to verify and approve.

Added in a new function that will reset the camera to x:0, y:0 and avoid
the user having to use set_x/y_camera functions to reset the position to
(0,0)

The function was added to the coresponding unit test and works with no
new errors
Added what the equivilent of this function would be into the
documentation
Included the `<cassert>` header and added in checks that the camera was
reseting to (0,0) everytime the test was complete
@olive-tree-branch

Copy link
Copy Markdown
Author

Thanks for the feedback.

I've added in some checks for the integration test to make sure that the positions are (0,0) after the reset_camera() function was called. The test itself allows the camera to be moved so I didn't add anything to make the camera move specifically.

Not sure how drawing_options.cpp got there in the first place but I have removed it from the PR.

@jankiluitel jankiluitel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for addressing the feedback. The camera position checks have now been added, and the unrelated drawing_options.cpp changes have been removed. The implementation and test coverage look good from my side. Approved. ✅

@Nadiazahan Nadiazahan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work on this. The previous feedback has been addressed and the tests now check that both the camera X and Y positions return to 0 after reset_camera() is called. The unrelated changes have also been removed, so the PR is focused on the new camera function.

One small non-blocking suggestion: in the documentation, maybe change “centre position on the screen” to “camera position (0, 0)” just to make it clearer what the function actually resets to.

Other than that, everything looks good to me. Approved.

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.

3 participants