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 ColorMatrix tint method #921

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mathieuanthoine
Copy link

To get the exact Adobe Animate tint, you need to remove the LUMA factor from the tint method.

@PrimaryFeather
Copy link
Contributor

PrimaryFeather commented Nov 14, 2016

Thanks a lot for the pull request, and sorry for the late reply (I was on a vacation).

May I ask where you've got this information from? I must admit the existing code was just copied from here, and I haven't checked if the output is identical to what Adobe Animate produces.

The problem with this change is that it will change the color output of all all existing users of this method, which might be problematic.

What do others say? I'd prefer to have the same output as Adobe Animate, but I'm not too happy changing people's tinting. 😐

@mathieuanthoine
Copy link
Author

May I ask where you've got this information from? I must admit the existing code was just copied from here, and I haven't checked if the output is identical to what Adobe Animate produces.

I've checked it myself, comparing graphic results, your and my code.

The problem with this change is that it will change the color output of all all existing users of this method, which might be problematic.

You're right except if developers that use it expect the same result as in Animate.

The safer solution is certainly to change the documentation for tint removing the description and create an editorTint method with the description you have at the moment for tint :)

@PrimaryFeather
Copy link
Contributor

Thanks for the update!

To everybody else in the loop: what's your opinion on this? Any heavy users of this method around?

@binouze
Copy link

binouze commented Nov 16, 2016

I used tint in m'y previous game an i remember having problems to get the same result as in flash pro. I think the solution to create an another method is good, so no need to find every usage of the tint method to retune it on the next update :)

@mathieuanthoine
Copy link
Author

I found strange behaviors in Starling. Using the same shader in PixiJs and Starling, the result is different.
I think that the shader is the right one but in some case the result is unexpected.. Please don't merge this PR before I found why the results in Starling are not as we could expected. Thanks.

@PrimaryFeather
Copy link
Contributor

Thanks for the feedback, @binouze!

@mathieuanthoine: What you're describing might be a difference in how PixiJS handles premultiplied alpha. In Starling 2, all colors are expected to use premultiplied alpha, and I'm making up for that in the shader. I don't know what PixiJS does in this respect. (Could be something else entirely, of course — I just thought I mention it.)

@mathieuanthoine
Copy link
Author

I'm not as solid as I would like about Shaders in Starling/AS3, how can I help to fix the tint method ?

@PrimaryFeather
Copy link
Contributor

@mathieuanthoine Do you mean that in PixiJS, the results are identical with those from Adobe Animate, but Starling produces different results (even with your changes)?

@mathieuanthoine
Copy link
Author

@PrimaryFeather Not exactly
I'll try to be clearer.

If in PixiJS I use the shader I give you for Starling, you will have the same tint as Animate.
In Starling it works in some cases (that's why at the beginning I thought that it works for Starling and PixiJS) and doesn't work in other cases.

I think the best way to check if the shader is the good one would be to apply the shader on a classic MovieClip in Animate.

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.

3 participants