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

COMP: Fix -Wunused-parameter in ctkVTKRenderView::lookFromAxis() #999

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Oct 21, 2021

The fov parameter became obsolete following d620ad9 (BUG: Fix
unexpected camera position change in ctkVTKRenderView::lookFromAxis)

@jcfr jcfr force-pushed the fix-ctkVTKRenderView-warning branch 2 times, most recently from 82eceb5 to dd07ecf Compare October 21, 2021 08:15
@jcfr
Copy link
Member Author

jcfr commented Oct 21, 2021

It looks like we should update the docstring to document the API change introduced in d620ad9 and explain the fov parameter has no effect:

/// \brief Change camera to look from a given axis to the focal point
/// Translate/Rotate the camera to look from a given axis
/// The Field of View (fov) controls how far from the focal point the
/// camera must be (final_pos = focal_point + 3*fov).
void lookFromAxis(const ctkAxesWidget::Axis& axis, double fov = 10.);

@Sunderlandkyl Could you suggest an updated docstring ?

@Sunderlandkyl
Copy link
Contributor

I don't remember making this commit, so I'm not sure what changed.
Maybe @lassoan made the change from the Mac laptop while I was still signed into Git there?

@lassoan
Copy link
Member

lassoan commented Oct 21, 2021

I've made that change and indeed the FOV parameter is not used anymore. It was not a good idea to force changing the camera distance from the focal point when the user only requested a view direction change.

I think the cleanest solution would be to have two versions of this method:

  • void lookFromAxis(const ctkAxesWidget::Axis& axis, double fov); -> deprecated, log a warning message when used, remove it in a few years
  • void lookFromAxis(const ctkAxesWidget::Axis& axis);

@jcfr jcfr force-pushed the fix-ctkVTKRenderView-warning branch from dd07ecf to aa0ef38 Compare May 5, 2022 05:57
@jcfr jcfr requested a review from lassoan May 5, 2022 05:58
jcfr and others added 2 commits October 24, 2022 13:50
The fov parameter became obsolete following d620ad9 (BUG: Fix
unexpected camera position change in ctkVTKRenderView::lookFromAxis)
This commit is a follow up of d620ad9 (BUG: Fix unexpected camera
position change in ctkVTKRenderView::lookFromAxis) removing the use
of the "fov" parameter originally introduced in fbb33cd (Add
ctkVTKRenderView::lookFromAxis(axis, fov))

It was not a good idea to force changing the camera distance from
the focal point when the user only requested a view direction
change.

Co-authored-by: Andras Lasso <lasso@queensu.ca>
@jcfr jcfr force-pushed the fix-ctkVTKRenderView-warning branch from aa0ef38 to 8e2d15d Compare October 24, 2022 17:51
@jcfr
Copy link
Member Author

jcfr commented Oct 24, 2022

Since all comments have been addressed, I plan on merging after the checks are complete and ✔️ .

@jcfr jcfr merged commit 65a0555 into commontk:master Oct 24, 2022
@jcfr jcfr deleted the fix-ctkVTKRenderView-warning branch October 24, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants