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

opacity binding #1972

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

19jrushlow
Copy link
Contributor

The binding works, but the value displayed in the cheat sheet does not update until some other binding has been called by the user. I suspect the issue lies in docDblOpt, but I cannot figure it out and would like someone to review the problem.

…isplayed on the cheatsheet, it only updates when another binding is called
@snoyer
Copy link
Contributor

snoyer commented Feb 10, 2025

@19jrushlow

The binding works, but the value displayed in the cheat sheet does not update until some other binding has been called by the user. I suspect the issue lies in docDblOpt, but I cannot figure it out and would like someone to review the problem.

I don't think it's your code, the cheat sheet is only updated if it has been invalidated, and it doesn't look like it is (yet) when changing the opacity, see

void vtkF3DRenderer::SetLightIntensity(const double intensityFactor)
{
if (this->LightIntensity != intensityFactor)
{
this->LightIntensity = intensityFactor;
this->LightIntensitiesConfigured = false;
this->CheatSheetConfigured = false;
}
}
vs
void vtkF3DRenderer::SetOpacity(const std::optional<double>& opacity)
{
if (this->Opacity != opacity)
{
this->Opacity = opacity;
this->ActorsPropertiesConfigured = false;
}
}


@mwestphal this // clang-format off looks a bit early

// clang-format off

should it go just before all the this->addBindings instead?

also should the bindings added by this PR go on the P key instead of T to match the existing "Toggle Translucency" binding? And should increasing/decreasing opacity turn translucency support on if it was off?

@mwestphal
Copy link
Contributor

it doesn't look like it is (yet) when changing the opacity, see

@snoyer is right, please add this->CheatSheetConfigured = false; in the method he pointed.

@mwestphal
Copy link
Contributor

should clang-format go just before all the this->addBindings instead?

Agreed

@mwestphal
Copy link
Contributor

should the bindings added by this PR go on the P key instead of T to match the existing "Toggle Translucency" binding?

Good point

And should increasing/decreasing opacity turn translucency support on if it was off?

I think that make sense but I would like @Meakk input on this

…w invalidates the cheat sheet in order to allow the binding to properly update the number displayed on the cheat sheet
@19jrushlow
Copy link
Contributor Author

19jrushlow commented Feb 11, 2025

The bug appears to be fixed after doing the cheat sheet invalidation thing, and I have switched the binding to P. It seems like all that should be left is:

  1. Add unit tests
  2. Possibly turn on translucency support when calling the binding, pending input from Meakk

Am I missing anything? Here is the original issue

@mwestphal
Copy link
Contributor

LGTM. just that one test to fix.

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

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

You also need to update the doc, doc/user/INTERACTIONS.md

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