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

Enable MeshShape to override alpha value #1315

Merged
merged 6 commits into from
May 16, 2019
Merged

Enable MeshShape to override alpha value #1315

merged 6 commits into from
May 16, 2019

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented May 14, 2019

This PR fixes that it was not able to change alpha value when the color mode of MeshShape is MATERIAL_COLOR.


Before creating a pull request

  • Document new methods and classes
  • Format new code files using clang-format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change (N/A)

@jslee02 jslee02 added this to the DART 6.9.0 milestone May 14, 2019
@jslee02 jslee02 marked this pull request as ready for review May 14, 2019 04:37
@mxgrey
Copy link
Member

mxgrey commented May 14, 2019

I'm not sure how I feel about adding an AlphaMode concept to the MeshShape. I suspect that would make it harder for users to alter transparency to their meshes, because they'd have to iterate through their shapes and find which shapes are of the type MeshShape and then toggle the value for that shape.

What if instead we always multiply the mesh material's alpha value by the VisualAspect's alpha value? That way if the VisualAspect has an alpha value of 1.0, then the rendering will just have the normal material alpha value (so materials like glass can still be transparent), and the closer the VisualAspect's alpha value gets to 0.0, the more transparent the mesh will be, until it's invisible.

@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #1315 into master will increase coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1315      +/-   ##
==========================================
+ Coverage   57.12%   57.12%   +<.01%     
==========================================
  Files         366      366              
  Lines       27055    27052       -3     
==========================================
  Hits        15454    15454              
+ Misses      11601    11598       -3
Impacted Files Coverage Δ
dart/dynamics/MeshShape.cpp 66.12% <0%> (+1.56%) ⬆️

@jslee02
Copy link
Member Author

jslee02 commented May 14, 2019

I see your point. I think this is because setting alpha (and the color) of MeshShape is already tricky. Users could have different expectations, and it seems your proposal is another behavior (maybe I call it blending mode?). One corner case I can think of is that it cannot handle the case that the user wants to set 1.0 alpha when the embedded alpha is lower than 1.0. So I wanted to make it explicit the behavior.

I would go with any better way as long as it can handle all the cases. My bottom line is that we should be able to set any value of the mesh alpha regardless of the color mode.

If we go with adding alpha mode, then I'm open to the default mode.

This is a little off topic, but I would like to add another color mode to MeshShape: tinting (or blending) the embedded mesh color (and texture) with the shape color. I haven't tested it myself recently but according to #739 we already allow it unintentionally. It would be good to make it explicit in that mode so that we can also disable it. One good use case of tinting is that tinting a skeleton or a mesh that is in a collision.

@mxgrey
Copy link
Member

mxgrey commented May 15, 2019

If we go with adding alpha mode, then I'm open to the default mode.

That sounds like a reasonable compromise. We could have the "alpha blend" mode be the default while still having a mode selection available from the MeshShape interface.

My only concern with loading so many "modes" into the MeshShape is that (by design) the specific shape type of a Shape instance is usually opaque to the user without manually inspecting the types, and I think that tends to make those APIs less accessible to users, to the point that a user might not realize that such functionality is available.

My ideal preference would be to have a way to manage these settings from the base Shape type, but I admit that I don't have a good suggestion for how to do that, since these modes are so specific to MeshShape.

I would like to add another color mode to MeshShape: tinting

I can definitely see the value of that 👍

@jslee02
Copy link
Member Author

jslee02 commented May 16, 2019

Updated based on the comments. Hope the changes agrees with the compromise.

My ideal preference would be to have a way to manage these settings from the base Shape type

Agree. I also couldn't come up with a better solution. Let's revisit this once we have more neat solution.

I saved the tinting option for future PRs.

Copy link
Member

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

Thanks! Hopefully this makes transparency a lot easier for users 👍

@jslee02 jslee02 merged commit dabcef0 into master May 16, 2019
@jslee02 jslee02 deleted the mesh_alpha branch May 16, 2019 04:24
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.

2 participants