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

Week 6 blog post #641

Merged
merged 5 commits into from Jul 28, 2022
Merged

Week 6 blog post #641

merged 5 commits into from Jul 28, 2022

Conversation

m-agour
Copy link
Contributor

@m-agour m-agour commented Jul 25, 2022

image

Copy link
Contributor

@ganimtron-10 ganimtron-10 left a comment

Choose a reason for hiding this comment

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

Hello @m-agour ,
(Welcome to Week 6 Party 🎉)
Nice Blog! I had a few suggestions mentioned below, PTAL.
Thanks!


- Improved the ``PlaybackPanel`` by adding speed control and the ability to loop the animation. Also, fixed the lagging issue of play and pause buttons and composed them into a single play/pause button.

- Updated the old tutorials' syntax to match the other tutorials and added a new tutorial on position animation using spline interpolation. And added unit tests for the ``PlaybackPanel`` and the newly added color converters in ``colormap.py``.
Copy link
Contributor

Choose a reason for hiding this comment

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

extra ' in tutorials'


- Added more hooks to the 2D sliders to cover two more states: ``on_value_changed`` and ``on_moving_slider`` `#634`_.

- Provided the ability to add static actors to the ``Timeline``, which might be needed in the animation part of shivam's glTF project.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe Shivam's


- Improved the ``PlaybackPanel`` by adding speed control and the ability to loop the animation. Also, fixed the lagging issue of play and pause buttons and composed them into a single play/pause button.

- Updated the old tutorials' syntax to match the other tutorials and added a new tutorial on position animation using spline interpolation. And added unit tests for the ``PlaybackPanel`` and the newly added color converters in ``colormap.py``.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these lines should be something like this
... spline interpolation and added unit tests ...
or
... spline interpolation. Added unit tests ...

Copy link
Member

@xtanion xtanion left a comment

Choose a reason for hiding this comment

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

Hi @m-agour, Nice blog. I have a few small suggestions PTAL.
Thanks


- A custom evaluator uses a user-provided function that takes time as input and evaluates the property at that time. This feature is yet to be discussed more in today's meeting.

- Fixed camera rotation, and view-up issue when interacting with the scene.
Copy link
Member

Choose a reason for hiding this comment

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

, not needed after rotation


- Provided the ability to add static actors to the ``Timeline``, which might be needed in the animation part of shivam's glTF project.

- If an ``actor`` is added to the ``Timeline`` as a static actor, it won't be animated by the ``Timeline``, but it will get added to the scene along with the ``Timeline`` when the ``Timeline`` is added to the scene.
Copy link
Member

Choose a reason for hiding this comment

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

This sentence is too long. (for e.g. You can remove when the ``Timeline`` is added to the scene and it won't make much difference in the sentence)

@m-agour
Copy link
Contributor Author

m-agour commented Jul 25, 2022

Thanks @ganimtron-10 and @xtanion so much for reviewing my blog post.


- Updated the old tutorials' syntax to match the other tutorials and added a new tutorial on position animation using spline interpolation. Added unit tests for the ``PlaybackPanel`` and the newly added color converters in ``colormap.py``.

- Added more hooks to the 2D sliders to cover two more states: ``on_value_changed`` and ``on_moving_slider`` `#634`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

What is difference? why do you need to add them? can you explain more here


- Added more hooks to the 2D sliders to cover two more states: ``on_value_changed`` and ``on_moving_slider`` `#634`_.

- Provided the ability to add static actors to the ``Timeline``, which might be needed in the animation part of Shivam's glTF project.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shivam's glTF project

add a link


What is coming up next week?
----------------------------
Next week's work is yet to be determined.
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems you can update now

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #641 (e570d71) into master (bca7b33) will decrease coverage by 1.02%.
The diff coverage is 0.00%.

❗ Current head e570d71 differs from pull request most recent head 8aa49c1. Consider uploading reports for the commit 8aa49c1 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
- Coverage   52.04%   51.02%   -1.03%     
==========================================
  Files         108      108              
  Lines       23812    23818       +6     
  Branches     2631     2631              
==========================================
- Hits        12394    12152     -242     
- Misses      11013    11256     +243     
- Partials      405      410       +5     
Impacted Files Coverage Δ
fury/ui/tests/test_containers.py 0.00% <0.00%> (ø)
fury/fury/tests/test_window.py 50.94% <0.00%> (-35.43%) ⬇️
fury/fury/ui/tests/test_containers.py 64.76% <0.00%> (-28.96%) ⬇️
fury/fury/window.py 72.85% <0.00%> (-10.41%) ⬇️
fury/fury/shaders/base.py 84.84% <0.00%> (-8.09%) ⬇️
fury/fury/ui/containers.py 82.92% <0.00%> (-0.89%) ⬇️
fury/fury/data/fetcher.py 67.81% <0.00%> (-0.43%) ⬇️
fury/fury/actor.py 88.87% <0.00%> (-0.19%) ⬇️
fury/fury/tests/test_interactor.py 89.76% <0.00%> (+0.08%) ⬆️
fury/fury/tests/test_actors.py 91.65% <0.00%> (+0.09%) ⬆️
... and 1 more

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 @m-agour.

See below my last comment


- The reason for adding these two hooks is that there was only the ``on_change`` hook, which always gets called when the value of the slider is changed without considering how the value is changed, hence, the functionality of the slider was limited.

- Provided the ability to add static actors to the ``Timeline``, which might be needed in the animation part of Shivam's glTF project `#634`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong link

@skoudoro
Copy link
Contributor

Thank you for the update @m-agour, merging

@skoudoro skoudoro merged commit 2ac1bb3 into fury-gl:master Jul 28, 2022
@m-agour
Copy link
Contributor Author

m-agour commented Jul 28, 2022

Thank you @skoudoro

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.

None yet

4 participants