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

[Enhance] Improve control of 3D visualisation of pose lifter #1481

Merged
merged 4 commits into from
Jul 12, 2022

Conversation

pallgeuer
Copy link
Contributor

@pallgeuer pallgeuer commented Jul 11, 2022

Motivation

When visualising the 3D output poses coming from a pose lifter, it is currently not possible to control the visualisation height (pixels) and viewpoint angle, even though both are already implemented just under the hood, and would only need to be patched through. The former can be important for instance if a higher resolution output is desired, or as in my case, if you require it to have the exact same pixel height as the input image sequence (for post-processing reasons). I have also noticed that the default viewpoint of the 3D visualisation is quite frontal, so in many cases the output looks correct, but if you look at it from a significantly different azimuth you can quickly tell it's not actually correct at all. In order to be able to properly evaluate and understand the outputs of various pose lifter models it is crucial to be able to visualise it from different viewing angles (I always use two views and plot them side by side for optimal understanding). Hence, why exposing axis_azimuth is quite important.

Modification

I added arguments to allow vis_height and axis_azimuth to be set when calling vis_3d_pose_result().
I also added some missing parameter documentation that I noticed was missing.

BC-breaking

Nothing.

Use cases

See motivation.

Checklist

Before PR:

  • I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
  • Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
  • New functionalities are covered by complete unit tests. If not, please add more unit tests to ensure correctness.
  • The documentation has been modified accordingly, including docstring or example tutorials.

After PR:

  • CLA has been signed and all committers have signed the CLA in this PR.

@jin-s13
Copy link
Collaborator

jin-s13 commented Jul 12, 2022

Thanks for your contribution!
It seems the unittest part fails. Could you please also take a look at it?

@jin-s13 jin-s13 requested a review from ly015 July 12, 2022 08:16
@pallgeuer
Copy link
Contributor Author

I had thought PoseLifter was the only model type that goes through that area of code, but Interhand3D indeed also does. So I have fixed that by adding the axis_azimuth parameter to Interhand3D as well. Note that the default value of axis_azimuth has to be None on the common pathway, because PoseLifter wants a default of 70 and Interhand3D wants a default of -115. I have preserved that behaviour.

@codecov
Copy link

codecov bot commented Jul 12, 2022

Codecov Report

Merging #1481 (60826a2) into master (bf9bd53) will not change coverage.
The diff coverage is n/a.

❗ Current head 60826a2 differs from pull request most recent head 77ffbda. Consider uploading reports for the commit 77ffbda to get more accurate results

@@           Coverage Diff           @@
##           master    #1481   +/-   ##
=======================================
  Coverage   84.47%   84.47%           
=======================================
  Files         236      236           
  Lines       20036    20036           
  Branches     3602     3602           
=======================================
  Hits        16925    16925           
  Misses       2231     2231           
  Partials      880      880           
Flag Coverage Δ
unittests 84.37% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
mmpose/apis/inference_3d.py 77.57% <ø> (ø)
mmpose/models/detectors/interhand_3d.py 60.29% <ø> (ø)
mmpose/models/detectors/pose_lifter.py 70.22% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeebc65...77ffbda. Read the comment docs.

@ly015
Copy link
Member

ly015 commented Jul 12, 2022

Thanks for your contribution. I made minor modifications to the default value of the parameters.

@ly015 ly015 changed the title Improve control of 3D visualisation of pose lifter [Enhance] Improve control of 3D visualisation of pose lifter Jul 12, 2022
@ly015 ly015 merged commit afb37d4 into open-mmlab:master Jul 12, 2022
@pallgeuer
Copy link
Contributor Author

Okay, so if I understand your commit correctly, the default internal value of axis_azimuth is now always 70, but the InterHand3D demo (the only place the vis method is ever called for InterHand?) specifies an explicit value of -115? And the InterHand3D class also tries to enforce a default value of -115, but this value is generally ignored anyway because the general vis method above it in the callstack already applied a default of 70 if no explicit axis_azimuth is specified?

@ly015
Copy link
Member

ly015 commented Jul 13, 2022

Yes,

Okay, so if I understand your commit correctly, the default internal value of axis_azimuth is now always 70, but the InterHand3D demo (the only place the vis method is ever called for InterHand?) specifies an explicit value of -115? And the InterHand3D class also tries to enforce a default value of -115, but this value is generally ignored anyway because the general vis method above it in the callstack already applied a default of 70 if no explicit axis_azimuth is specified?

Yes, that's correct. Although in our demos we always invoke the top-level visualization interfaces like vis_3d_pose_result(), it is not prohibited to directly use the lower-level functions like model.show_results(). So I think it also makes sense to keep a default parameter value for the latter. And we are also working on simplifying the encapsulation of inference and visualization APIs, which is a little bit deep indeed.

BTW, sorry for the hasty merge of this PR. If you have any concerns, we can open a new PR to further polish it.

@pallgeuer
Copy link
Contributor Author

No, I'm happy with the current merge, now that I have understood it.

wusize pushed a commit that referenced this pull request Jul 20, 2022
* Add some missing parameter documentation

* Expose vis_height / axis_azimuth

* Fix interhand axis_azimuth

* keep original default behavior

Co-authored-by: ly015 <liyining0712@gmail.com>
evendrow pushed a commit to evendrow/mmpose that referenced this pull request Dec 22, 2022
…lab#1481)

* Add some missing parameter documentation

* Expose vis_height / axis_azimuth

* Fix interhand axis_azimuth

* keep original default behavior

Co-authored-by: ly015 <liyining0712@gmail.com>
shuheilocale pushed a commit to shuheilocale/mmpose that referenced this pull request May 5, 2023
…lab#1481)

* Add some missing parameter documentation

* Expose vis_height / axis_azimuth

* Fix interhand axis_azimuth

* keep original default behavior

Co-authored-by: ly015 <liyining0712@gmail.com>
ajgrafton pushed a commit to ajgrafton/mmpose that referenced this pull request Mar 6, 2024
…lab#1481)

* Add some missing parameter documentation

* Expose vis_height / axis_azimuth

* Fix interhand axis_azimuth

* keep original default behavior

Co-authored-by: ly015 <liyining0712@gmail.com>
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.

None yet

3 participants