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

Cone actor uses repeat_primitive by default #547

Merged
merged 7 commits into from Apr 15, 2022

Conversation

xtanion
Copy link
Member

@xtanion xtanion commented Feb 18, 2022

Resolves issue #529

@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #547 (c6dec82) into master (9b3e3a1) will increase coverage by 0.10%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
+ Coverage   88.58%   88.69%   +0.10%     
==========================================
  Files          55       55              
  Lines       11003    11107     +104     
  Branches     1084     1092       +8     
==========================================
+ Hits         9747     9851     +104     
  Misses        959      959              
  Partials      297      297              
Impacted Files Coverage Δ
fury/fury/tests/test_primitive.py 100.00% <0.00%> (ø)
fury/fury/tests/test_actors.py 91.52% <0.00%> (+0.09%) ⬆️
fury/fury/actor.py 88.15% <0.00%> (+0.14%) ⬆️
fury/fury/primitive.py 94.94% <0.00%> (+1.30%) ⬆️

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 @xtanion,

There is some conflict in your branch. Can you resolve them?
Can you update the tutorial or add one.

look at this PR for an example

Thank you

fury/actor.py Outdated
@@ -1858,6 +1858,8 @@ def cone(centers, directions, colors, heights=1., resolution=10,
If faces is None then a cone is created based on directions, heights
and resolution. If not then a cone is created with the provided
vertices and faces.
use_primitive: boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

bool, optional instead


Parameters
----------
radius: float
Copy link
Contributor

Choose a reason for hiding this comment

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

float, optional instead

----------
radius: float
Radius of the cone
height: float
Copy link
Contributor

Choose a reason for hiding this comment

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

float, optional instead

Radius of the cone
height: float
Height of the cone
sectors: int
Copy link
Contributor

Choose a reason for hiding this comment

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

int, optional instead


"""

if not sectors > 2:
Copy link
Contributor

@skoudoro skoudoro Apr 13, 2022

Choose a reason for hiding this comment

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

if sectors < 2: instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @xtanion,

There is some conflict in your branch. Can you resolve them? Can you update the tutorial or add one.

look at this PR for an example

Thank you

Thanks for the review @skoudoro, I made the changes, please take a look.
Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the update. Unfortunately, tests are failing so I will check only after you fix the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @skoudoro, I just figured out that advanced_geometry_test was failing due to the arrow_actor. changing the height parameter with scale fixes the size of arrows 😅 .
I changed the axes actor's repeat_primitive=False, window.snapshot wasn't detecting colors while using primitive arrows.

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.

Thank you for the update. See below for additional comments.

scene.add(cone_actor1)
scene.add(cone_actor2)

interactive = True
Copy link
Contributor

Choose a reason for hiding this comment

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

False instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

if interactive:
window.show(scene, size=(600, 600))

window.record(scene, out_path='viz_markers.png', size=(600, 600))
Copy link
Contributor

Choose a reason for hiding this comment

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

why viz_markers ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wrote that by mistake, Modified to viz_cone.

fury/actor.py Outdated
colors=colors, active_scalars=heights,
source=src, vertices=vertices, faces=faces)
return cone_actor
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

else not needed

@skoudoro skoudoro linked an issue Apr 15, 2022 that may be closed by this pull request
@skoudoro skoudoro merged commit a19168c into fury-gl:master Apr 15, 2022
@skoudoro
Copy link
Contributor

Thank you @xtanion for this

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

Successfully merging this pull request may close these issues.

Cone actor needs to use repeat_primitives by default
2 participants