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

Removed the ceil'ing of width and height in base texture #5959

Merged
merged 3 commits into from Aug 1, 2019

Conversation

themoonrat
Copy link
Member

@themoonrat themoonrat commented Jul 27, 2019

As otherwise you lose precision going from one to the other.

Just trying to create some demo's that show up the issue in pixi playground

@themoonrat
Copy link
Member Author

Grrr, thought this would be an easy one to reproduce - will come back and have a fresh think about why this fixed an issue in my version of the libs and why it's not doing anything here.

@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Jul 27, 2019

As otherwise you lose precision going from one to the other.

Just trying to create some demo's that show up the issue in pixi playground

Grrr, thought this would be an easy one to reproduce - will come back and have a fresh think about why this fixed an issue in my version of the libs and why it's not doing anything here.

Welcome to the X-Files. I told you about extra eps before!

Please provide a demo , that way we'll fix it correctly and decide what to do with really fractional realWidth :)

@ivanpopelyshev ivanpopelyshev added the 🛸 X-Files Mysterious, unexplainable or elusive behavior that’s difficult to track down or reproduce. label Jul 27, 2019
@themoonrat
Copy link
Member Author

themoonrat commented Jul 28, 2019

The base64 encoded image is just a white square in the middle of a few other bits.

0.75x base texture asset, which doesn't work on v5
v5: https://www.pixiplayground.com/#/edit/wpSKMBzOgh1zvO6q~_iBw
v5 with this PR: https://www.pixiplayground.com/#/edit/RVnw2iNv3lVmSmNf5kBcA
v4: https://www.pixiplayground.com/#/edit/ZtI5gS5beIhU~UqRTx98b

0.5x base texture asset, which does work on v5
v5: https://www.pixiplayground.com/#/edit/rXVCUJrbeQqh8Ry0JWosb
v5 with this PR: https://www.pixiplayground.com/#/edit/iVypjXAmU9ATTjFxY8erW
v4: https://www.pixiplayground.com/#/edit/VLTcbSc5S41opxAJ9sGAM

@themoonrat
Copy link
Member Author

Tbf, in this test, even v4 gets the height wrong.... works fine with Texture.WHITE

@bigtimebuddy bigtimebuddy added this to the v5.1.1 milestone Jul 28, 2019
@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Jul 29, 2019

Good examples. My eps doesnt help there: https://www.pixiplayground.com/#/edit/VofrhNr_0CURPAIW2LB49

Also, resolution=0.9 works for filters just fine:
https://www.pixiplayground.com/#/edit/m~C_u2aCDfkFHBKoqjZYL

In general , yes, I believe that if we set real dimensions of texture , width/height should be fractional.

Good Job!

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.

Hold on guys, I found something.

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.

HEre we go: https://www.pixiplayground.com/#/edit/d~jTog12dqKjAY9K1wBmg

Look in the console.

We need "Math.ceil(x -eps)" in realWidth and realHeight getter. or just Math.round(). What do you think?

I suggest eps=1e-4.

@codecov-io
Copy link

codecov-io commented Aug 1, 2019

Codecov Report

Merging #5959 into dev will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev    #5959   +/-   ##
=======================================
  Coverage   70.43%   70.43%           
=======================================
  Files         208      208           
  Lines       10518    10518           
=======================================
  Hits         7408     7408           
  Misses       3110     3110
Impacted Files Coverage Δ
packages/core/src/textures/BaseTexture.js 90.06% <100%> (ø) ⬆️

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 a302f0c...d1d04e4. Read the comment docs.

@bigtimebuddy bigtimebuddy merged commit 9aefa01 into dev Aug 1, 2019
@bigtimebuddy bigtimebuddy deleted the dev-fix-basetexture-over-zealous-ceiling branch August 1, 2019 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛸 X-Files Mysterious, unexplainable or elusive behavior that’s difficult to track down or reproduce.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants