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

Texture rotation is broken for Canvas #8678

Closed
ivanpopelyshev opened this issue Sep 29, 2022 · 9 comments
Closed

Texture rotation is broken for Canvas #8678

ivanpopelyshev opened this issue Sep 29, 2022 · 9 comments

Comments

@ivanpopelyshev
Copy link
Collaborator

Original issue: pixijs/spine#460

After #8615, spine rotation doesnt work anymore.

I honestly dont know which version is wrong, I suspect is that packer in #8615 is wrong, because my version was supposed to work with Spine and TexturePacker.

I'm gonna check whether TexturePacker atlases work for us now, then revert that PR.

@ivanpopelyshev
Copy link
Collaborator Author

@smlmyck please tell me more about that free texture packer tool

@smlmyck
Copy link
Contributor

smlmyck commented Sep 29, 2022

@ivanpopelyshev We use a tool at the company I work for that wraps https://github.com/odrick/free-tex-packer-core to generate our texture atlases for runtime loading.

If I throw the assets used for my original demo through TexturePacker (using the "JSON (Hash)" exporter), the issue still occurs as written on PIXI.js 6.5.2, and is fixed by my change in #8615 (and by extension 6.5.3+).

I've spun up another Glitch site demonstrating that here: https://denim-boom-building.glitch.me/ - the image is identical and the JSON is effectively identical to the previous Glitch for my previous PR.

We've had no issues loading textures in this fashion in any PIXI.js version from 3.0.11 until 6.5.0 landed where the problem starting occurring for us.

@ivanpopelyshev
Copy link
Collaborator Author

ivanpopelyshev commented Sep 29, 2022

Maybe the actual issue is that it doesnt have "trim" , and only "frame" width/height shouldnt be rotated.

Can you check that in your case those lines arent hit in debugger ?

destWidth = texture.trim.width;
destHeight = texture.trim.height;

@smlmyck
Copy link
Contributor

smlmyck commented Sep 29, 2022

That's correct, it steps over that if block as texture.trim is null. I'll have a play around to see if I can resolve this.

@smlmyck
Copy link
Contributor

smlmyck commented Sep 29, 2022

Can confirm that I've broken texture atlases that use trimming, which for my repro (and what we saw in our projects) wasn't in use because everything rotated had pixels to the edge. Adding some transparent pixels make the texture packer trim them shows a broken canvas renderer as it was for non-trimmed atlases prior to #8615 landing.

Changing this if block

if (texture.trim)
{
destWidth = texture.trim.width;
destHeight = texture.trim.height;
}

... to fix the destWidth/Height conditionally if the texture is rotated does indeed fix both my original case in #8615 and the breakage caused by it, i.e. this:

        if (texture.trim)
        {
            if (texture.rotate) {
                destWidth = texture.trim.height;
                destHeight = texture.trim.width;
            } else {
                destWidth = texture.trim.width;
                destHeight = texture.trim.height;
            }
        }
        
I'll raise a PR for that shortly.

@ivanpopelyshev
Copy link
Collaborator Author

We support different rotations, so better use groupD8.isVertical(rotation) . https://pixijs.io/examples/#/textures/texture-rotate.js
Dont mind diagonal ones, its feature-bug

@ivanpopelyshev
Copy link
Collaborator Author

@bigtimebuddy can we have that in another v6 release please?

@bigtimebuddy
Copy link
Member

Sure, make the PR and I can make a new v6 release. This issue would also impact v7 too.

@bigtimebuddy
Copy link
Member

Released in 6.5.5.

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

No branches or pull requests

3 participants