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 bug fix for point and sphere actors #351

Merged
merged 2 commits into from Jan 6, 2021

Conversation

SunTzunami
Copy link
Member

Fixes #335

Earlier,
vr

Now,
vr2

Earlier, opacity argument wasn't manipulating the opacity of point actors and sphere actors. That issue is resolved by this fix. Unit tests are included for the same.

Copy link
Contributor

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Hi @SunTzunami,

It looks good to me. I just need to try it locally and then I will go ahead and merge your PR.

Thank you for this update

@skoudoro skoudoro added this to the v0.7.0 milestone Jan 5, 2021
@skoudoro skoudoro added the type:Bug Fix Something isn't working label Jan 5, 2021
@skoudoro
Copy link
Contributor

skoudoro commented Jan 6, 2021

After looking more in detail into your fix, I think we should avoid using actor.GetProperty().SetOpacity(opacity). It seems not compatible with our architecture.
Ideally, in the function actor.sphere, we would check if opacity != 1. if yes, we should update the color array by adding a dimension if it is an Nx3, or by updating the last dimension if it is an Nx4.

I will merge your PR because it fixes the actor.point but we need a new PR to improve this version.

@skoudoro skoudoro merged commit 1e320d8 into fury-gl:master Jan 6, 2021
@skoudoro
Copy link
Contributor

skoudoro commented Jan 6, 2021

thank you @SunTzunami

@SunTzunami
Copy link
Member Author

Hi @skoudoro. To make these fixes, I read the documentation for line actor (https://fury.gl/latest/_modules/fury/actor.html#line) as transparency worked for line actor. Line actor too uses actor.GetProperty().SetOpacity(opacity) to manipulate opacity so I assumed it'll be fine to use that for sphere and point actors too.

@skoudoro
Copy link
Contributor

skoudoro commented Jan 6, 2021

In both cases, it needs to be improved @SunTzunami.

if you look here, the 3 spheres have 3 differents opacities : 0.3, 04, 0.99. This is how we setup the opacities. actor.GetProperty().SetOpacity(opacity) do not update this array and override our values. Each object can have a different opacity which is impossible with actor.GetProperty().SetOpacity(opacity).

@SunTzunami
Copy link
Member Author

@skoudoro thanks for the info. I'll try making a better fix.

@SunTzunami SunTzunami deleted the opacity branch February 15, 2021 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_opacity argument for point doesn't seem to work
2 participants