Integrate scalar quantity along a stream or path line #2893

[StasJ]

Adds support for integrating scalar value along a stream or path line. Allows the user to select an integration region in which the integration occurs. The integration region can be controlled with an in-visualizer manipulator like the flow rake. When using the integration mode, the TF histogram will correctly show a new histogram for the integrated values rather than the scalar field. The TF Editor mapping range slider default range will change to the range of integrated values as well.

Tested with forward, backward, bi-directional, steady, unsteady, 2D, and 3D flows.

Fix #2893

[clyne]

Awesome, @StasJ! Has Matthias and/or Annie had a chance to try this out to make sure it's doing what they want? Let's do that before we review. Thanks!

[sgpearse]

Interesting feature and the code looks good as always stas. I have two comments:

1. How come the "Integration Region" is different from our normal region? I think they should be the same but Maybe I'm missing something.

2. Can we put the contents of the Integration tab in Appearance? These settings let the user apply color/opacity to a new variable and I think this fits best next to the Transfer Function. This sounds like an important feature, but I don't think it should be presented to the user as a top-level tab.

IMO, the Appearance tab could have a drop-down listing "Use Constant Color, Color Mapped Variable, and Color Integrated Variable" or something like that.

[clyne]

Here are some thoughts I had on improving GUI presentation of this new capability, the primary problem being introducing a new "integration" tab that seems out of place and potentially confusing to the majority of users who will not want to make use of this feature.

In my view the "integration" feature is simply another way of assigning color values along the path/stream lines. It's really not much different than the two color modes we currently support (constant, and color by value). Rather than having a separate "integration" tab I propose the following changes on the "appearance" tab:

1. Change the "Use constant color" checkbox to a dropdown labeled "Color mode" with options:

• By value
• Constant
• Integral along curve
• Total integral along curve

All of the modes above, with the exception of constant, would use the "Color mapped variable" (no need for a separate "scalar to integrate")

2. Hide the remaining integration controls (integration distance and integration region) when one of the two integration modes isn't selected.

Thoughts, @sgpearse, @shaomeng, @StasJ?

[sgpearse]

@clyne I think what you're proposing makes sense but the dilemma is the hackery going on in the VizWin. This problem seems to be unclear, so here's my understanding of it.

In VizWin.cpp there is a function called _getCurrentMouseMode() which determines whether we draw the Box manipulators. Drawing the manipulators is based on what tab is currently active, determined by GUIStateParams.

I believe your proposal would work if we added logic to _getCurrentMouseMode() that checks whether we're doing flow integration, like the following:

string VizWin::_getCurrentMouseMode()
{
    ParamsMgr *     paramsMgr = _controlExec->GetParamsMgr();
    GUIStateParams *guiP = (GUIStateParams *)paramsMgr->GetParams(GUIStateParams::GetClassType());

    bool showIntegrationBox = false;
    VAPoR::RenderParams *rParams = _getRenderParams();
    FlowParams* fParams = dynamic_cast<VAPoR::FlowParams*>(rParams);
    if (fParams != nullptr) {
        if (fParams->GetValueLong(FlowParams::_doIntegrationTag, false))
            showIntegrationBox=true;
    }

    string activeTab = guiP->ActiveTab();
    if (activeTab == RenderEventRouterGUI::GeometryTabName ||
        activeTab == FlowEventRouter::SeedingTabName ||
        showIntegrationBox)
        return MouseModeParams::GetRegionModeName();
    else
        return MouseModeParams::GetNavigateModeName();
}

I believe this kind of hackery is what Stas is trying to avoid.

I'm personally ok with doing this, because the VizWin isn't clean in the first place and this gets the job done. We should probably revisit the entire file. But if another hack on an already hacked system helps the users, then I think it's temporarily worth it.

For what it's worth, I think the Renderer base class should draw the manipulators, not logic in the VizWin or GUI.

[StasJ]

Here are some thoughts I had on improving GUI presentation of this new capability, the primary problem being introducing a new "integration" tab that seems out of place and potentially confusing to the majority of users who will not want to make use of this feature.

In my view the "integration" feature is simply another way of assigning color values along the path/stream lines. It's really not much different than the two color modes we currently support (constant, and color by value). Rather than having a separate "integration" tab I propose the following changes on the "appearance" tab:

1. Change the "Use constant color" checkbox to a dropdown labeled "Color mode" with options:

• By value
• Constant
• Integral along curve
• Total integral along curve

All of the modes above, with the exception of constant, would use the "Color mapped variable" (no need for a separate "scalar to integrate")

2. Hide the remaining integration controls (integration distance and integration region) when one of the two integration modes isn't selected.

Thoughts, @sgpearse, @shaomeng, @StasJ?

I agree, however like Scott said, the reason it was implemented this way is because of legacy code for the visualizer manipulator that needs either refactoring or ugly hacks to make your suggestion work.

[shaomeng]

Thoughts, @sgpearse, @shaomeng, @StasJ?

I understood this new GUI interaction and think it makes sense. I wasn't aware of the implementation difficulties though.

[clyne]

Thanks for the clarification. The PR has been approved. I'm going to merge it. @StasJ can you please open a Usability/Refactoring issue to address the above (either as proposed, or by some other solution)?

[StasJ]

Thanks for the clarification. The PR has been approved. I'm going to merge it. @StasJ can you please open a Usability/Refactoring issue to address the above (either as proposed, or by some other solution)?

I opened an issue here: #2964

[clyne]

Thanks!