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

Isolate Selected Geometries (master branch) #7587

Merged
merged 2 commits into from Feb 16, 2017

Conversation

Projects
None yet
7 participants
@ellensi
Contributor

ellensi commented Feb 9, 2017

Purpose

This pull request is a better reimplementation of my 2016 recharge project previously addressed by #6610. The functionality was formerly called "Selective Geometry Preview".

A new menu item called Isolate Selected Geometries is added into the Settings menu.

When the mode is enabled, only selected nodes will have their corresponding geometries displayed in full opacity. Geometries that correspond to unselected nodes will be displayed transparent with 10% opacity for meshes and 20% opacity for lines and points.

selective

Improvements

NEW implementation Old implementation
Transparency flag is passed to the Dynamo shader (more efficient) Alpha values were recalculated on C# for each mesh triangle (inefficient)
Lines and points can be displayed transparent with 20% opacity Lines and points were not made transparent yet
Axes and grids are visible among the transparent geometries Axes and grids were covered behind the supposedly transparent geometries
"Isolate Selected Geometries" menu item is under Settings menu "Selective Geometry Preview" dummy button was located to the left of the Geom/Node toggle button

Limitations

  • Semi-transparent Plane geometries when selected may cause some unselected transparent geometries to be rendered wrongly
  • Changing the "Show edges" setting while under selective geometry preview mode will reset the transparency. As a workaround, user can just click at any blank space of the node canvas.
  • Direct Manipulation functionality cannot work properly behind transparent geometries
  • Possible confusion with frozen geometries

Declarations

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate - TBA
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@Benglin @Racel

FYIs

@riteshchandawar

@ikeough

This comment has been minimized.

Show comment
Hide comment
@ikeough

ikeough Feb 9, 2017

Contributor

@ellensi This is really cool. The transparent draw-order problem is a known issue. We never implemented efficient transparency sorting to take care of that.

Contributor

ikeough commented Feb 9, 2017

@ellensi This is really cool. The transparent draw-order problem is a known issue. We never implemented efficient transparency sorting to take care of that.

Show outdated Hide outdated src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs
// selected items under selective geometry preview mode will have higher precedence
var selectedA = AttachedProperties.GetSelectivePreview(a) &&
!AttachedProperties.GetShowSelected(a) && !AttachedProperties.IsSpecialRenderPackage(a);
var selectedB = AttachedProperties.GetSelectivePreview(a) &&

This comment has been minimized.

@Benglin

Benglin Feb 10, 2017

Contributor

This might be a typo GetSelectivePreview(a)

@Benglin

Benglin Feb 10, 2017

Contributor

This might be a typo GetSelectivePreview(a)

@riteshchandawar

This comment has been minimized.

Show comment
Hide comment
@riteshchandawar

riteshchandawar Feb 10, 2017

Contributor

@Racel Please take a look at this and suggest few things

  1. Location of Button
  2. Name of functionality.
Contributor

riteshchandawar commented Feb 10, 2017

@Racel Please take a look at this and suggest few things

  1. Location of Button
  2. Name of functionality.
@Racel

This comment has been minimized.

Show comment
Hide comment
@Racel

Racel Feb 10, 2017

Contributor

@ellensi - What happens when frozen nodes are included in the mix? How do you distinguish between frozen and selected geometry?

If "Selective Geometry Preview" is off, selecting individual nodes turns them blue? Why don't they turn blue under this mode? Can you clarify as this is a bit confusing.

Additionally, I am not 100% sure about adding another toggle button to the canvas, but I would like to hear your thoughts on why this decision was made. The geometry/node toggle we have now is already confusing as it is. This button introduces a new UX paradigm that is different from the existing toggle (you turn it off/on as opposed to switching between 2 buttons). While I understand that this is to make the feature more discoverable and easier to access, we have existing paradigms for this sort of thing (enabling-features) already with things like Show Edges, Show Preview Bubbles, Show Connectors, Show Grid. Is there a reason why this feature is getting special attention? Additionally, could we use existing examples that have similar functionality...Like in Revit or 3dsmax isn't this similar to "isolate"? Maybe we could use that for the name of the functionality and follow that UX/UI precedent for it? In the new Web UI, to accomodate things like this and make it easier for people to access settings, we may need something like a settings bar...

Thoughts?

Contributor

Racel commented Feb 10, 2017

@ellensi - What happens when frozen nodes are included in the mix? How do you distinguish between frozen and selected geometry?

If "Selective Geometry Preview" is off, selecting individual nodes turns them blue? Why don't they turn blue under this mode? Can you clarify as this is a bit confusing.

Additionally, I am not 100% sure about adding another toggle button to the canvas, but I would like to hear your thoughts on why this decision was made. The geometry/node toggle we have now is already confusing as it is. This button introduces a new UX paradigm that is different from the existing toggle (you turn it off/on as opposed to switching between 2 buttons). While I understand that this is to make the feature more discoverable and easier to access, we have existing paradigms for this sort of thing (enabling-features) already with things like Show Edges, Show Preview Bubbles, Show Connectors, Show Grid. Is there a reason why this feature is getting special attention? Additionally, could we use existing examples that have similar functionality...Like in Revit or 3dsmax isn't this similar to "isolate"? Maybe we could use that for the name of the functionality and follow that UX/UI precedent for it? In the new Web UI, to accomodate things like this and make it easier for people to access settings, we may need something like a settings bar...

Thoughts?

@ellensi

This comment has been minimized.

Show comment
Hide comment
@ellensi

ellensi Feb 13, 2017

Contributor

@Racel

On frozen geometries: Under this new functionality, frozen geometries will be displayed 50% transparent if selected (that is the normal alpha for frozen geometries) and 5% transparent if unselected (that is 50% of 10%). UX suggestions will be more than welcome.

On selected geometry color: I personally prefer the blue overlay to be turned off as I feel that it gives a matt effect that blurs the depth perception of geometries. I'd like to hear others' suggestions for this.

image

About this tool's name: "Isolate Selected Geometries", or any better idea?

About a new toggle button: @riteshchandawar and I have discussed about the current geometry/node toggle button that does not look like a single toggle button. Initially I intended to merge that toggle into something that looks like a single button while adding this new "isolate" button to its left side, but those two new compacted toggle buttons will surely confuse our existing users. While I believe that a settings toolbar will be a good improvement, we're planning to finalize this "isolate" functionality for Dynamo 1.3. How would you finalize the design for now? Should we put it into the Settings menu?

cc: @riteshchandawar @kronz

Contributor

ellensi commented Feb 13, 2017

@Racel

On frozen geometries: Under this new functionality, frozen geometries will be displayed 50% transparent if selected (that is the normal alpha for frozen geometries) and 5% transparent if unselected (that is 50% of 10%). UX suggestions will be more than welcome.

On selected geometry color: I personally prefer the blue overlay to be turned off as I feel that it gives a matt effect that blurs the depth perception of geometries. I'd like to hear others' suggestions for this.

image

About this tool's name: "Isolate Selected Geometries", or any better idea?

About a new toggle button: @riteshchandawar and I have discussed about the current geometry/node toggle button that does not look like a single toggle button. Initially I intended to merge that toggle into something that looks like a single button while adding this new "isolate" button to its left side, but those two new compacted toggle buttons will surely confuse our existing users. While I believe that a settings toolbar will be a good improvement, we're planning to finalize this "isolate" functionality for Dynamo 1.3. How would you finalize the design for now? Should we put it into the Settings menu?

cc: @riteshchandawar @kronz

@riteshchandawar

This comment has been minimized.

Show comment
Hide comment
@riteshchandawar

riteshchandawar Feb 13, 2017

Contributor

@Racel During recharge project we put this option in Setting Menu only, but to make to more discoverable, we made it as a button.

Current Geo/Node buttons are confusing as both doesn't the same job, we should combine those together and only show the other one when the first one is active. For example, When user is in Node View then button will be for changing it to Geometry View and vice versa, so the single button will serve the two purpose.

But as @ellensi pointed out, if we make that consolidation of existing button and add the new one adjacent to it, then you might get confused between old Geometry button and the new one.

Thoughts?

Contributor

riteshchandawar commented Feb 13, 2017

@Racel During recharge project we put this option in Setting Menu only, but to make to more discoverable, we made it as a button.

Current Geo/Node buttons are confusing as both doesn't the same job, we should combine those together and only show the other one when the first one is active. For example, When user is in Node View then button will be for changing it to Geometry View and vice versa, so the single button will serve the two purpose.

But as @ellensi pointed out, if we make that consolidation of existing button and add the new one adjacent to it, then you might get confused between old Geometry button and the new one.

Thoughts?

@kronz

This comment has been minimized.

Show comment
Hide comment
@kronz

kronz Feb 13, 2017

Contributor

@ellensi @Racel my concern is that we do not add a new UI widget unless we need to. Seems like the current model for global view settings in the View>Background 3d Preview menu.

another question: how does this interact with he Revit preview? My assumption would be that if you are using this new feature, and Revit Preview is enabled, then the selected elements also show up in Revit.

Contributor

kronz commented Feb 13, 2017

@ellensi @Racel my concern is that we do not add a new UI widget unless we need to. Seems like the current model for global view settings in the View>Background 3d Preview menu.

another question: how does this interact with he Revit preview? My assumption would be that if you are using this new feature, and Revit Preview is enabled, then the selected elements also show up in Revit.

@Racel

This comment has been minimized.

Show comment
Hide comment
@Racel

Racel Feb 13, 2017

Contributor

@ellensi @riteshchandawar @kronz - Here are my suggestions:

  • Let's get rid of the canvas button for now and put it back in the "Settings Menu".

  • For the name, maybe we should have a setting called "Isolate Selected Geometry"? I could see users always wanting this on or off. So, in a way, it makes sense in the Settings Menu.

  • If possible, can you give me a branch build with the button in the canvas? I would like to do some testing on it. Also, if this feature goes into master, will we be able to call this feature in the web UI and test from there if we just add a button?

Contributor

Racel commented Feb 13, 2017

@ellensi @riteshchandawar @kronz - Here are my suggestions:

  • Let's get rid of the canvas button for now and put it back in the "Settings Menu".

  • For the name, maybe we should have a setting called "Isolate Selected Geometry"? I could see users always wanting this on or off. So, in a way, it makes sense in the Settings Menu.

  • If possible, can you give me a branch build with the button in the canvas? I would like to do some testing on it. Also, if this feature goes into master, will we be able to call this feature in the web UI and test from there if we just add a button?

@mjkkirschner

This comment has been minimized.

Show comment
Hide comment
@mjkkirschner

mjkkirschner Feb 14, 2017

Contributor

@Racel - on the last bullet, I am pretty sure no, since these changes are almost entirely in wpf and in shaders. It will need to be re-implemented in threejs / webGL... if we were to re-implement this at all, or Formit might need to reimplement functionality like this in their viewer if we are to share that going forward?

Contributor

mjkkirschner commented Feb 14, 2017

@Racel - on the last bullet, I am pretty sure no, since these changes are almost entirely in wpf and in shaders. It will need to be re-implemented in threejs / webGL... if we were to re-implement this at all, or Formit might need to reimplement functionality like this in their viewer if we are to share that going forward?

@Racel

This comment has been minimized.

Show comment
Hide comment
@Racel

Racel Feb 14, 2017

Contributor

Thanks @mjkkirschner - I see what you mean. If that is the case, the more I think that this feature should live in the "Settings Menu" for now

Additionally, we should have a keyboard shortcut to toggle it on/off. Please ensure that it does not conflict with any of the existing keyboard shortcuts or the ones proposed in https://jira.autodesk.com/browse/DYN-433

Contributor

Racel commented Feb 14, 2017

Thanks @mjkkirschner - I see what you mean. If that is the case, the more I think that this feature should live in the "Settings Menu" for now

Additionally, we should have a keyboard shortcut to toggle it on/off. Please ensure that it does not conflict with any of the existing keyboard shortcuts or the ones proposed in https://jira.autodesk.com/browse/DYN-433

@@ -27,7 +27,8 @@ float2 vShadowMapSize = float2(1024, 1024);
float4 vShadowMapInfo = float4(0.005, 1.0, 0.5, 0.0);
float4 vSelectionColor = float4(0.0, 0.62, 1.0, 1.0);
float4 vTransparentMaterial = 0.1f;
float4 vTransparentLinePoint = 0.2f;

This comment has been minimized.

@Benglin

Benglin Feb 16, 2017

Contributor

Does this have the same effect as the following?

float4 vTransparentLinePoint = float4(0.2f, 0.2f, 0.2f, 0.2f);

@Benglin

Benglin Feb 16, 2017

Contributor

Does this have the same effect as the following?

float4 vTransparentLinePoint = float4(0.2f, 0.2f, 0.2f, 0.2f);

Show outdated Hide outdated src/DynamoCoreWpf/ViewModels/Watch3D/DynamoGeometryModel3D.cs
var isSelected = (bool)GetValue(AttachedProperties.ShowSelectedProperty);
var selectiveMode = (bool)GetValue(AttachedProperties.SelectivePreviewProperty);
var isSpecialPackage = (bool)GetValue(AttachedProperties.IsSpecialRenderPackageProperty);

This comment has been minimized.

@Benglin

Benglin Feb 16, 2017

Contributor

Few notes for the above...

  1. To align the name with this feature, should selectiveMode be isolationMode? That may avoid the confusion due to another isSelected variable above.
  2. Not sure what isSpecialPackage mean, can we rename it to inheritsTransparencyValue or something like that? All we can do as readers here is to guess its meaning.

Also some comments above these lines should help describe it better (and also redirect other files' comments to point to this file).

@Benglin

Benglin Feb 16, 2017

Contributor

Few notes for the above...

  1. To align the name with this feature, should selectiveMode be isolationMode? That may avoid the confusion due to another isSelected variable above.
  2. Not sure what isSpecialPackage mean, can we rename it to inheritsTransparencyValue or something like that? All we can do as readers here is to guess its meaning.

Also some comments above these lines should help describe it better (and also redirect other files' comments to point to this file).

Show outdated Hide outdated src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs
var selectedB = AttachedProperties.GetSelectivePreview(b) &&
!AttachedProperties.GetShowSelected(b) && !AttachedProperties.IsSpecialRenderPackage(b);
result = selectedA.CompareTo(selectedB);
if (result != 0) return result;

This comment has been minimized.

@Benglin

Benglin Feb 16, 2017

Contributor

Please help with documenting these change as discussed, thanks!

@Benglin

Benglin Feb 16, 2017

Contributor

Please help with documenting these change as discussed, thanks!

@Benglin

This comment has been minimized.

Show comment
Hide comment
@Benglin

Benglin Feb 16, 2017

Contributor

All else being addressed, LGTM, thanks!

Contributor

Benglin commented Feb 16, 2017

All else being addressed, LGTM, thanks!

@ellensi ellensi merged commit eba3276 into DynamoDS:master Feb 16, 2017

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
EngOps Build EngOps Build Succeeded
Details

ellensi added a commit to ellensi/Dynamo that referenced this pull request Feb 16, 2017

Implement selective geometry preview using shaders (#7587)
* Implement selective geometry preview using shaders

* Move "Isolate Selected Geometries" into Settings menu

ellensi added a commit that referenced this pull request Feb 16, 2017

Implement selective geometry preview using shaders (#7587) (#7614)
* Implement selective geometry preview using shaders

* Move "Isolate Selected Geometries" into Settings menu

@ellensi ellensi changed the title from Implement selective geometry preview using shaders to Isolate Selected Geometries (master branch) Feb 17, 2017

@ellensi

This comment has been minimized.

Show comment
Hide comment
@ellensi

ellensi Feb 20, 2017

Contributor

We merged this in early so that the functionality can be tested in the bug bash. The feature name is set to be "Isolate Selected Geometries" and it is under the Settings menu. Keyboard shortcut (probably Shift+S) will be added in a follow-up pull request. As for now, the selected geometries will stay gray in color. Thanks everyone for your input!

Contributor

ellensi commented Feb 20, 2017

We merged this in early so that the functionality can be tested in the bug bash. The feature name is set to be "Isolate Selected Geometries" and it is under the Settings menu. Keyboard shortcut (probably Shift+S) will be added in a follow-up pull request. As for now, the selected geometries will stay gray in color. Thanks everyone for your input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment