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

Fixes roundPixels calculation differences between v4 and v5 #6234

Merged
merged 4 commits into from Dec 14, 2019

Conversation

themoonrat
Copy link
Member

@themoonrat themoonrat commented Nov 21, 2019

Description of change

Fixes issue descripted in #6232 with roundPixels implementation difference between v4 and v5 for resolutions less than 1.

Pre-Merge Checklist
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@themoonrat
Copy link
Member Author

Added a PR using the PIXI.settings.RESOLUTION settings.....
But that's not ideal if a different resolution is passed directly into the renderer.

So help on a rethink of this is required please! :)

@ivanpopelyshev
Copy link
Collaborator

Can you add this to settings.RESOLUTION description?

Btw, we have separate RESOLUTION and FILTER_RESOLUTION

@ivanpopelyshev
Copy link
Collaborator

I'm happy that you solved it for me. I didnt know what to do with that thing back then when I moved it to calculateVertices()

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

@themoonrat could you possibly add a unit-test or two for this? It's a very subtle thing, but has some implications visually. Would be helpful to make sure we handle that case correctly in the future.

@bigtimebuddy bigtimebuddy added this to the v5.2.1 milestone Nov 24, 2019
@codecov-io
Copy link

codecov-io commented Dec 4, 2019

Codecov Report

Merging #6234 into dev will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6234      +/-   ##
==========================================
- Coverage   77.14%   77.13%   -0.02%     
==========================================
  Files         203      203              
  Lines       10314    10316       +2     
==========================================
  Hits         7957     7957              
- Misses       2357     2359       +2
Impacted Files Coverage Δ
packages/mesh/src/Mesh.js 61.48% <0%> (-0.42%) ⬇️
packages/sprite/src/Sprite.js 92.15% <0%> (-0.46%) ⬇️

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 eab9585...98eef3b. Read the comment docs.

@themoonrat
Copy link
Member Author

@bigtimebuddy Sure, I'll have time on Friday to see if this can be unit tested in a meaningful way. Hopefully I can look in the vertex data to explain why putting 2 store sprites side by side does not work without this PR

@bigtimebuddy bigtimebuddy merged commit 6a12035 into dev Dec 14, 2019
@bigtimebuddy bigtimebuddy deleted the dev-round-pixels-fix branch December 14, 2019 22:01
@bigtimebuddy
Copy link
Member

Thanks @themoonrat if you could add a unit-test PR later, that'd be great.

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