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

Octagonal Prism and Frustum Square Pyramid #249

Merged
merged 31 commits into from
Jun 19, 2020
Merged

Conversation

mlraglin
Copy link
Contributor

@mlraglin mlraglin commented Jun 2, 2020

Added two new primitive shapes: the octagonal prism and the frustum square pyramid, to primitive.py and actor.py. Also added tests for both new shapes in test_primitive.py and test_actor.py.

@pep8speaks
Copy link

pep8speaks commented Jun 2, 2020

Hello @mlraglin! 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 2020-06-12 17:11:16 UTC

@skoudoro skoudoro added this to the v0.6.0 milestone Jun 2, 2020
@skoudoro
Copy link
Contributor

skoudoro commented Jun 2, 2020

Hi @mlraglin,

Thank you for these new features! Can you address pep8 errors when you have time? Furthermore, test_vertices_primitives_octagonalprism does not pass. Can you run and check it on your environment to see what's going on?

I'm tagging @lenixlobo and @Nibba2018 to review your PR after your update.

Thank you !

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

codecov bot commented Jun 3, 2020

Codecov Report

Merging #249 into master will decrease coverage by 0.14%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
- Coverage   88.69%   88.55%   -0.15%     
==========================================
  Files          17       17              
  Lines        4387     4446      +59     
  Branches      553      569      +16     
==========================================
+ Hits         3891     3937      +46     
- Misses        356      366      +10     
- Partials      140      143       +3     
Impacted Files Coverage Δ
fury/actor.py 93.12% <100.00%> (+0.36%) ⬆️
fury/primitive.py 87.06% <100.00%> (+1.35%) ⬆️
fury/utils.py 86.79% <0.00%> (-3.24%) ⬇️
fury/ui.py 86.13% <0.00%> (-0.16%) ⬇️
fury/data/__init__.py 100.00% <0.00%> (ø)
fury/interactor.py 96.92% <0.00%> (+0.53%) ⬆️

@lenixlobo

This comment has been minimized.

fury/tests/test_actors.py Show resolved Hide resolved
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 @mlraglin.

I will wait for @lenixlobo and @Nibba2018 last review before merging this PR.

fury/tests/test_primitive.py Outdated Show resolved Hide resolved
fury/actor.py Outdated Show resolved Hide resolved
@lenixlobo
Copy link
Contributor

Great job!
Looks good to me!

@skoudoro skoudoro merged commit da86080 into fury-gl:master Jun 19, 2020
@skoudoro
Copy link
Contributor

Thank you @mlraglin! Merging

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.

5 participants