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

Bug: New 🌈 Color change broke pixi-spine tint #9277

Closed
SerG-Y opened this issue Mar 17, 2023 · 9 comments · Fixed by pixijs-userland/spine#492 or #9280
Closed

Bug: New 🌈 Color change broke pixi-spine tint #9277

SerG-Y opened this issue Mar 17, 2023 · 9 comments · Fixed by pixijs-userland/spine#492 or #9280
Labels
🕷 Bug Verified that it’s actually a legit bug that exists in the current release.

Comments

@SerG-Y
Copy link
Contributor

SerG-Y commented Mar 17, 2023

Current Behavior

It's hard to explain :) Actually, it's better to compare the images.
I am pretty sure it's related to these two lines in SpineBase:
https://github.com/pixijs/spine/blob/7903e9dae81a855a6725d13789f31ca70260ede8/packages/base/src/SpineBase.ts#L328
https://github.com/pixijs/spine/blob/7903e9dae81a855a6725d13789f31ca70260ede8/packages/base/src/SpineBase.ts#L386
Do you have any idea why it broke pixi-spine?

image

Expected Behavior

image

Steps to Reproduce

I think it's easy to reproduce with any spine project that has tint stuff

Environment

  • pixi.js version: 7.2.0
  • pixi-spine version: 4.0.3

Possible Solution

No response

Additional Information

No response

@bigtimebuddy
Copy link
Member

@SerG-Y we need a repo on this. I tried using the public examples and applying a tint and it did what I would expect.

@bigtimebuddy bigtimebuddy added the 👯‍♀️ Needs Reproduction Very common: basically need an example that reproduces the issue so that it's easier to identify. label Mar 17, 2023
@SerG-Y
Copy link
Contributor Author

SerG-Y commented Mar 17, 2023

@bigtimebuddy I will try to investigate it a bit more if not I will back with repo.
but changing core.utils.rgb2hex to old rgb2hex made a trick
function rgb2hex(rgb) { return (((rgb[0] * 255) << 16) + ((rgb[1] * 255) << 8) + (rgb[2] * 255 | 0)); }

@SerG-Y
Copy link
Contributor Author

SerG-Y commented Mar 20, 2023

Hi @bigtimebuddy, can you please tag someone who can take a look at my PR. Thanks!

@bigtimebuddy
Copy link
Member

I think the better fix here is replace this._value !== value with a deep-equality. It seems totally reasonable to re-use an object, liken an array and re-set the Color value.

@SerG-Y
Copy link
Contributor Author

SerG-Y commented Mar 20, 2023

@bigtimebuddy I think a deep-equality will not help here. Because this._value is a reference to an array and when you change a value in an array it also will be changed in this._value as it's a reference. So, with deep-equality result will be the same. Did I miss something?

@SuperSodaSea
Copy link
Collaborator

Maybe we can deep copy the value when setting it.

@SerG-Y
Copy link
Contributor Author

SerG-Y commented Mar 20, 2023

@SuperSodaSea sounds good!

@bigtimebuddy bigtimebuddy reopened this Mar 20, 2023
@SuperSodaSea SuperSodaSea added 🕷 Bug Verified that it’s actually a legit bug that exists in the current release. and removed 👯‍♀️ Needs Reproduction Very common: basically need an example that reproduces the issue so that it's easier to identify. labels Mar 20, 2023
@SerG-Y
Copy link
Contributor Author

SerG-Y commented Mar 21, 2023

@miltoncandelero in this case can you please revert my commit from pixi-spine as it's not needed anymore.

@FloodGames
Copy link

Deprecates the following color utilities (#9061) @bigtimebuddy
utils.rgb2hex => new Color(rgb).toNumber()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕷 Bug Verified that it’s actually a legit bug that exists in the current release.
Projects
None yet
4 participants