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

Changing dot actor #584

Merged
merged 10 commits into from May 17, 2022
Merged

Changing dot actor #584

merged 10 commits into from May 17, 2022

Conversation

tvcastillod
Copy link
Contributor

Hello, I made this PR in order to improve flexibility of the current dot actor. I changed dots actor to allow usage of multiple colors, so now it has the option to set a different color for each point created. For it to support both ndarray (N,3 or 4) and tuple (3 or 4,) I created a function called __color_check to manage colors dot's parameter. Additionally, as the actor includes an opacity parameter If a value is given, each dot will have the same opacity otherwise opacity is set to 1 by default, or is defined by Alpha parameter in colors if given.

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #584 (5260726) into master (add8cc0) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
+ Coverage   87.95%   87.96%   +0.01%     
==========================================
  Files          62       62              
  Lines       13030    13099      +69     
  Branches     1308     1316       +8     
==========================================
+ Hits        11460    11522      +62     
- Misses       1198     1201       +3     
- Partials      372      376       +4     
Impacted Files Coverage Δ
fury/fury/actor.py 88.28% <0.00%> (-0.39%) ⬇️
fury/fury/tests/test_actors.py 91.40% <0.00%> (-0.27%) ⬇️
fury/fury/material.py 74.60% <0.00%> (ø)
fury/fury/actors/peak.py 94.25% <0.00%> (ø)
fury/fury/shaders/tests/test_base.py 93.08% <0.00%> (+0.03%) ⬆️
fury/fury/utils.py 91.75% <0.00%> (+0.08%) ⬆️
fury/fury/shaders/base.py 92.92% <0.00%> (+0.14%) ⬆️
fury/fury/tests/test_utils.py 94.61% <0.00%> (+0.34%) ⬆️
fury/fury/data/fetcher.py 72.41% <0.00%> (+1.37%) ⬆️

Copy link
Contributor

@guaje guaje left a comment

Choose a reason for hiding this comment

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

@tvcastillod Thanks for this PR. The new dot actor looks promising. And the color_check function can benefit many actors. Here are some comments and sugestions.

fury/tests/test_actors.py Outdated Show resolved Hide resolved
fury/tests/test_actors.py Outdated Show resolved Hide resolved
fury/tests/test_actors.py Outdated Show resolved Hide resolved
fury/tests/test_actors.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
Copy link
Contributor

@guaje guaje left a comment

Choose a reason for hiding this comment

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

@tvcastillod thank you for applying the previously requested changes, the addition of the test_color_check function, and the unique check.

I've added some comments regarding the latest changes.

Additionally, you need to rebase the PR.

fury/utils.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
fury/utils.py Show resolved Hide resolved
fury/utils.py Show resolved Hide resolved
fury/utils.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented May 13, 2022

Hello @tvcastillod! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-05-17 02:20:22 UTC

fury/actor.py Outdated Show resolved Hide resolved
@guaje
Copy link
Contributor

guaje commented May 17, 2022

LGTM! Thanks @tvcastillod. Merging

@guaje guaje merged commit 38e9c1f into fury-gl:master May 17, 2022
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.

None yet

3 participants