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

Continuing glTF/PBR Implementation #854

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

m-agour
Copy link
Contributor

@m-agour m-agour commented Dec 16, 2023

This PR is based on the previous work in PR #715 . Trying to improve the PBR-based glTF models rendering in FURY.

It's been some time since my last contribution, and I'm excited to jump back in.
Currently, the PBR materials are working but without Environmental lighting and reflection as in the screenshot below:

image

I propose merging this to establish a solid ground from which we move on with more advanced lighting for the PBR glTF models.

@pep8speaks
Copy link

pep8speaks commented Dec 16, 2023

Hello @m-agour, Thank you for updating!

Line 279:80: E501 line too long (84 > 79 characters)
Line 439:80: E501 line too long (84 > 79 characters)
Line 459:80: E501 line too long (80 > 79 characters)
Line 465:80: E501 line too long (84 > 79 characters)

Line 188:80: E501 line too long (84 > 79 characters)
Line 189:80: E501 line too long (84 > 79 characters)
Line 190:80: E501 line too long (88 > 79 characters)

To test for issues locally, pip install flake8 and then run flake8 fury.

Comment last updated at 2024-06-11 19:32:34 UTC

@m-agour
Copy link
Contributor Author

m-agour commented Dec 16, 2023

So, Tests are failing even though objects are being rendered, I suspect its because I modified the lightening so when it tests for a specific color it might not be there anymore. i.e. this duck produced by the test code triggers an error even though it seems fine.
image

@m-agour m-agour closed this Dec 16, 2023
@m-agour m-agour reopened this Dec 16, 2023
@m-agour m-agour marked this pull request as draft December 16, 2023 06:19
@skoudoro
Copy link
Contributor

Thank you for this @m-agour.

I will be able to look into this only after December 27. I will keep you updated.

Thanks again!

@m-agour
Copy link
Contributor Author

m-agour commented Dec 18, 2023

Hello @skoudoro, please take your time. I'll keep enhancing this it and troubleshooting the tests meanwhile.
Happy holidays!

Copy link

codecov bot commented May 31, 2024

Codecov Report

Attention: Patch coverage is 54.32099% with 37 lines in your changes missing coverage. Please review.

Project coverage is 84.08%. Comparing base (b38afc1) to head (d326f75).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #854      +/-   ##
==========================================
- Coverage   84.37%   84.08%   -0.29%     
==========================================
  Files          44       44              
  Lines       10553    10620      +67     
  Branches     1432     1445      +13     
==========================================
+ Hits         8904     8930      +26     
- Misses       1270     1302      +32     
- Partials      379      388       +9     
Files Coverage Δ
fury/gltf.py 76.08% <54.32%> (-3.50%) ⬇️

…ted scene

Revert "fix: test updated to check for color match between exported and imported scene"

This reverts commit 67096f5.

fix: test updated to check for color match between exported and imported scenes
@m-agour m-agour force-pushed the Fixing-PBR-for-glTF-loader branch from 67096f5 to 15d200d Compare June 8, 2024 01:28
@m-agour m-agour marked this pull request as ready for review June 8, 2024 17:40
@skoudoro
Copy link
Contributor

Hi @m-agour,

Can you fix the 2 CI's "Codespell" and "Code format" before starting the review. Maybe a rebase of this PR will be enought to fix it, I did not check. Thank you in advance

@m-agour
Copy link
Contributor Author

m-agour commented Jun 11, 2024

Hi @skoudoro , I will rebase today and ping you after all tests pass

@m-agour m-agour force-pushed the Fixing-PBR-for-glTF-loader branch from 9f7c6c2 to 01cfa15 Compare June 11, 2024 11:41
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.

4 participants