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

fix for sync uniform group issue #6217

Merged
merged 7 commits into from Dec 4, 2019
Merged

fix for sync uniform group issue #6217

merged 7 commits into from Dec 4, 2019

Conversation

GoodBoyDigital
Copy link
Member

Description of change

The introduction of syncData is to solves an issue where textures in uniform groups are not set correctly the texture count was always starting from 0 in each group. This needs to increment each time a texture is used no matter which group is being used.

Pre-Merge Checklist
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Nov 10, 2019

Trolling mode on: is there any way we can cover it with a test?

@bigtimebuddy
Copy link
Member

I agree with Ivan. Unit test this if we can

@ivanpopelyshev
Copy link
Collaborator

if you make playground test, i'll make unit based on it.

@GoodBoyDigital
Copy link
Member Author

cool! make an example 👍

@GoodBoyDigital
Copy link
Member Author

@ivanpopelyshev
Copy link
Collaborator

Good test.

Uncaught ReferenceError: triangle2 is not defined

Im making a test based on it.

Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev left a comment

Choose a reason for hiding this comment

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

That's how its done. Basically, I copied parts of your playground and I check for boundTextures. It fails at current dev version.

@ivanpopelyshev
Copy link
Collaborator

Please remove package.lock changes.

@codecov-io
Copy link

codecov-io commented Nov 12, 2019

Codecov Report

Merging #6217 into dev will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6217      +/-   ##
==========================================
+ Coverage   76.71%   76.73%   +0.01%     
==========================================
  Files         202      202              
  Lines       10238    10238              
==========================================
+ Hits         7854     7856       +2     
+ Misses       2384     2382       -2
Impacted Files Coverage Δ
packages/core/src/shader/ShaderSystem.js 93.58% <100%> (+0.16%) ⬆️
...ages/core/src/shader/utils/generateUniformsSync.js 86.2% <100%> (-0.89%) ⬇️
packages/core/src/shader/Shader.js 62.96% <0%> (+7.4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bacd8c...fa5ac5e. Read the comment docs.

@GoodBoyDigital
Copy link
Member Author

nice thanks man!

@bigtimebuddy bigtimebuddy added this to the v5.2.1 milestone Nov 13, 2019
@bigtimebuddy bigtimebuddy merged commit 80032dd into dev Dec 4, 2019
@bigtimebuddy bigtimebuddy deleted the uniform-fix branch December 4, 2019 04:52
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.

None yet

4 participants