Skip to content

fix: Hovering issues#104

Merged
AlejandroFernandezLuces merged 5 commits intomainfrom
fix/hover-followup
Jul 22, 2024
Merged

fix: Hovering issues#104
AlejandroFernandezLuces merged 5 commits intomainfrom
fix/hover-followup

Conversation

@AlejandroFernandezLuces
Copy link
Copy Markdown
Collaborator

@AlejandroFernandezLuces AlejandroFernandezLuces commented Jul 17, 2024

This addresses comments from @germa89 in #98
Hovering should be smoother now:

video.webm

@github-actions github-actions Bot added the bug Something isn't working label Jul 17, 2024
@AlejandroFernandezLuces AlejandroFernandezLuces marked this pull request as ready for review July 17, 2024 11:36
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 60.46%. Comparing base (7643914) to head (76e130b).

Files Patch % Lines
...isualization_interface/backends/pyvista/pyvista.py 60.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #104      +/-   ##
==========================================
- Coverage   60.53%   60.46%   -0.08%     
==========================================
  Files          24       24              
  Lines         821      817       -4     
==========================================
- Hits          497      494       -3     
+ Misses        324      323       -1     

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

@RobPasMue RobPasMue requested a review from germa89 July 17, 2024 12:04
Copy link
Copy Markdown
Contributor

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

I have some questions regarding this.

If self._object_to_actors_map is a general mapper for all the objects you plot, this object won't avoid to replot the legend with each interaction (mouse movement).

You should add another internal variable (a list?, a set?), where you will store the labels already plotted. then when you go into the if in line 318, the next thing is to check if that label has been already plotted by checking if it is stored in the mentioned internal variable. If it is not, plot; otherwise, continue.

I hope I'm reading your code and approach well!

Comment thread src/ansys/tools/visualization_interface/backends/pyvista/pyvista.py
Comment thread src/ansys/tools/visualization_interface/backends/pyvista/pyvista.py
Comment thread src/ansys/tools/visualization_interface/backends/pyvista/pyvista.py
Comment thread src/ansys/tools/visualization_interface/backends/pyvista/pyvista.py Outdated
@germa89
Copy link
Copy Markdown
Contributor

germa89 commented Jul 17, 2024

By the way, I cannot see the video :(

image

@germa89
Copy link
Copy Markdown
Contributor

germa89 commented Jul 17, 2024

By the way, I cannot see the video :(

image

It seems safari is very picky with webm.

@AlejandroFernandezLuces
Copy link
Copy Markdown
Collaborator Author

I have some questions regarding this.

If self._object_to_actors_map is a general mapper for all the objects you plot, this object won't avoid to replot the legend with each interaction (mouse movement).

You should add another internal variable (a list?, a set?), where you will store the labels already plotted. then when you go into the if in line 318, the next thing is to check if that label has been already plotted by checking if it is stored in the mentioned internal variable. If it is not, plot; otherwise, continue.

I hope I'm reading your code and approach well!

I agree with you, I already have a list with the plotted labels, the one I delete.

I tried the approach you propose by having a dict with the main actor as key, and with the label actor as value, so I can know if the current actor already has a label or not. But it doesn't seem to be working for some reason. It looks like it's saving a copy in the dict instead of a reference to the original object, so I can't delete the label afterwards.

Before sinking more time in this, I think the current implementation works fine despite this small inefficiency. Since we are always replotting only one label at a time, this issue won't scale with more actors, so it should be fine. What do you think?

@AlejandroFernandezLuces
Copy link
Copy Markdown
Collaborator Author

By the way, I cannot see the video :(

I wasn't aware that Safari couldn't visualize Webm, I converted the video to MP4.

video.mp4

@germa89
Copy link
Copy Markdown
Contributor

germa89 commented Jul 22, 2024

It looks like it's saving a copy in the dict instead of a reference to the original object, so I can't delete the label afterwards.

That's interesting.

Before sinking more time in this, I think the current implementation works fine despite this small inefficiency. Since we are always replotting only one label at a time, this issue won't scale with more actors, so it should be fine. What do you think?

I agree with your reasoning. Yes. Do not waste more time in that then.

I wasn't aware that Safari couldn't visualize Webm, I converted the video to MP4.

Thank you for taking the time!

Copy link
Copy Markdown
Contributor

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

LGTM!

@AlejandroFernandezLuces AlejandroFernandezLuces enabled auto-merge (squash) July 22, 2024 12:10
@AlejandroFernandezLuces AlejandroFernandezLuces merged commit 447cbff into main Jul 22, 2024
@AlejandroFernandezLuces AlejandroFernandezLuces deleted the fix/hover-followup branch July 22, 2024 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants