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 11 blog post #675

Merged
merged 3 commits into from Sep 6, 2022
Merged

Week 11 blog post #675

merged 3 commits into from Sep 6, 2022

Conversation

m-agour
Copy link
Contributor

@m-agour m-agour commented Aug 30, 2022

My blog post for week 11

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #675 (0afbb0f) into master (6caaa03) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 0afbb0f differs from pull request most recent head eda1393. Consider uploading reports for the commit eda1393 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
- Coverage   50.40%   50.39%   -0.01%     
==========================================
  Files         120      120              
  Lines       26980    26980              
  Branches     2986     2986              
==========================================
- Hits        13598    13597       -1     
  Misses      12923    12923              
- Partials      459      460       +1     
Impacted Files Coverage Δ
fury/fury/stream/tools.py 90.94% <0.00%> (-0.22%) ⬇️

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.

Nice Blog @m-agour ,
PTAL at my below comments.
Thanks!


- Find out how to get the ``Scene`` from the actor instead of manually assigning it.

- If I have time, I will try implement recording animation as GIF or as a video.
Copy link
Contributor

Choose a reason for hiding this comment

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

try to implement instead


- Fixed some issues in the hierarchical order animation support `PR`_ that we discussed during last week's meeting (mostly naming issues).

- Explained the introductory tutorial a little. But it is not good suitable beginners. So, I will spend time improving tutorials this week.
Copy link
Contributor

Choose a reason for hiding this comment

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

add for or rephrase the sentence
something like But it is not good for beginners.

@ganimtron-10
Copy link
Contributor

Ohh Sorry I just saw this PR is still a draft!
Maybe this review would help while working on it.

@m-agour m-agour marked this pull request as ready for review September 1, 2022 13:47
@m-agour m-agour requested a review from xtanion September 1, 2022 13:47
@m-agour
Copy link
Contributor Author

m-agour commented Sep 1, 2022

Ohh Sorry I just saw this PR is still a draft! Maybe this review would help while working on it.

It's okay

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.

Nice blog post @m-agour, No typos or any grammatical mistakes. PTAL at my suggestion below.


- Added extrusion to `vector_text`_ to allow the z-scaling to be functional.

- Fixed ``lightColor0`` being `hard-set`_ to ``(1, 1, 1)``. Instead, it's now using the ``Scene`` to set the lighting uniforms.
Copy link
Member

Choose a reason for hiding this comment

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

It uses instead of It's now using

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.

See below my comment. after this fix, it is ready to go. please answer to @xtanion also

What did you do this week?
--------------------------

This week I didn't do much work.
Copy link
Contributor

Choose a reason for hiding this comment

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

good to be honest but not sure we want to read this without explanation. so either explain or remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skoudoro done

@skoudoro
Copy link
Contributor

skoudoro commented Sep 6, 2022

thank you for the update, merging

@skoudoro skoudoro merged commit 22157d7 into fury-gl:master Sep 6, 2022
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