Add interactive bind to cycle backface culling modes - #3126
Conversation
mwestphal
left a comment
There was a problem hiding this comment.
looks good, please add tests and doc :)
| std::string CachePath; | ||
|
|
||
| std::optional<std::string> BackfaceType; | ||
| std::map<vtkProperty*, bool> OriginalBackfaceCulling; |
There was a problem hiding this comment.
why do you need a bool on top of the property ?
There was a problem hiding this comment.
Because the vtkProperty acts as a key for each material. We need the bool as a backup so when we cycle back to default, we can fully recover the original state for each part of the object.
There was a problem hiding this comment.
Im really confused. the original vtkProperty should contain all the info you need, should it not ?
|
|
||
| vtkProperty* prop = coloring.Actor->GetProperty(); | ||
| vtkProperty* origProp = coloring.OriginalActor->GetProperty(); | ||
| if (this->OriginalBackfaceCulling.count(prop) == 0) |
There was a problem hiding this comment.
find should be enough, no need to use count
|
Need any help moving forward @isaccunha ? :) |
|
I just pushed some changes, do you think it is ready and I can move on to testing and docs? |
Ill take a look :) |
mwestphal
left a comment
There was a problem hiding this comment.
Still unclear in regards to the vtkProperty thing.
Lets discuss on discord if you can :)
|
Need any help moving forward @isaccunha ? |
|
I am doing the refactor changes to the bug solution, but I'm still kinda lost, so I'll message you on discord.
|
|
Hey @isaccunha , I dont think you messaged me on discord, do not hesitate to ping me there! |
Yes I did, I am icee on discord, sorry to confuse you. Also I pushed the changes and think it is finally okay and the bug is solved. Do you think I can add docs and testing? |
|
Style Checks CI failed: diff --git a/library/src/interactor_impl.cxx b/library/src/interactor_impl.cxx
index 42cb041..644669e 100644
--- a/library/src/interactor_impl.cxx
+++ b/library/src/interactor_impl.cxx
@@ -1003,7 +1003,7 @@ interactor& interactor_impl::initCommands()
}
else if (*type == "visible")
{
- type = "hidden";
+ type = "hidden";
}
else if (*type == "hidden")
{
@@ -1013,7 +1013,7 @@ interactor& interactor_impl::initCommands()
},
command_documentation_t{
"cycle_backface_type", "cycle between backface culling modes (default, visible, hidden)" });
-
+
std::vector<std::string> cycleColoringValidArgs = { "field", "array", "component" };
this->addCommand(
"cycle_coloring",
@@ -1590,7 +1590,7 @@ interactor& interactor_impl::initBindings()
const auto& type = this->Internals->Options.render.backface_type;
if (!type.has_value())
{
- desc = "default";
+ desc = "default";
}
else
{You can copy the patch above and apply it locally with:
|
610e28a to
40bf38b
Compare
| std::string CachePath; | ||
|
|
||
| std::optional<std::string> BackfaceType; | ||
|
|
| this->addBinding({mod_t::NONE, "P"}, "cycle render.effect.blending.mode", "Scene", std::bind(docStr, "Blending", std::cref(opts.render.effect.blending.mode)), f3d::interactor::BindingType::CYCLIC); | ||
| this->addBinding({mod_t::NONE, "Q"}, "toggle render.effect.ambient_occlusion","Scene", std::bind(docTgl, "Ambient occlusion", std::cref(opts.render.effect.ambient_occlusion)), f3d::interactor::BindingType::TOGGLE); | ||
| this->addBinding({mod_t::NONE, "A"}, "cycle render.effect.antialiasing.mode","Scene", std::bind(docStr, "Anti-aliasing", std::cref(opts.render.effect.antialiasing.mode)), f3d::interactor::BindingType::CYCLIC); | ||
| this->addBinding({mod_t::CTRL, "B"},"cycle_backface_type","Scene",docBackface,f3d::interactor::BindingType::CYCLIC); |
There was a problem hiding this comment.
| this->addBinding({mod_t::CTRL, "B"},"cycle_backface_type","Scene",docBackface,f3d::interactor::BindingType::CYCLIC); | |
| this->addBinding({mod_t::CTRL, "B"}, "cycle_backface_type", "Scene", docBackface, f3d::interactor::BindingType::CYCLIC); |
| } | ||
| }; | ||
|
|
||
| // "Cycle backface type" , "visible/hidden/default" |
There was a problem hiding this comment.
f3d now supports "domains", you can drop this and declare a domain in options.json. See antialiasing mode for an example.
| "cycle_animation", "cycle scene.animation.index option using model information" }); | ||
|
|
||
| this->addCommand( | ||
| "cycle_backface_type", |
There was a problem hiding this comment.
f3d now supports "domains", you can drop this and declare a domain in options.json. See antialiasing mode for an example.
mwestphal
left a comment
There was a problem hiding this comment.
f3d changed since them , so you need to update the logic with domains, but other than that it looks good!
Describe your changes
This PR adds a command and a bind to control backface culling.
Issue ticket number and link if any
#2697
Checklist for finalizing the PR
.github/workflows/versions.json, I have updateddocker_timestampAI Disclosure
Continuous integration
Please write a comment to run CI, eg:
\ci fast.See here for more info.