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

Implement Matrix33 missing methods #134

Open
BigBadaboom opened this issue Dec 27, 2021 · 3 comments
Open

Implement Matrix33 missing methods #134

BigBadaboom opened this issue Dec 27, 2021 · 3 comments

Comments

@BigBadaboom
Copy link

BigBadaboom commented Dec 27, 2021

I need the missing methods for my project. So I have begun the work to implement them

I have completed most of the conversion work. I'm at the writing-unit-test stage.

But I have a few implementation questions first:

  1. I assume Matrix33 is intended to be the Skija version of SkMatrix. Correct? If so, I am curious. Why the name change?
  2. Is Matrix33 intended to be mutable or immutable?
  3. If mutable, I will need to remove Matrix33.IDENTITY, since something like Matrix.33.IDENTITY.setScale() will cause havoc.
  4. makePreScale() confuses me. It has the make prefix but it is not a static constructor. Was it intended that all the preScale()/, postScale() type fluent methods all be renamed to makePreScale()/makePostScale() style naming? Or is it just a mistake?
@tonsky
Copy link
Collaborator

tonsky commented Dec 27, 2021

Hi Paul!

  1. Skia added SkMatrix44 later, and I thought it would be great to have them consistently called. Now I see that Matrix44 is rarely needed, so maybe it wasn’t worth it
  2. Immutable by convention. Technically it does contain a mutable array, and there’s no attempts to prevent modifying it, only the convention.
  3. Immutable, so IDENTITY is ok
  4. I think the idea was that make means new object is created. But I agree it looks confusing, and limiting make to statics only makes sense.

Also, this repo is not supported anymore, new updates happen in https://github.com/HumbleUI/Skija/ only. I’d like to set a redirect but JetBrains is against it for some reason.

@BigBadaboom
Copy link
Author

Ok thanks. I'm using Skija as part of a compose-jb app I'm tinkering with. Will compose-jb eventually be switching to your fork? Is there a discussion somewhere I can read that explains the backstory?

@tonsky
Copy link
Collaborator

tonsky commented Dec 28, 2021

Compose forked Skija and auto-converted it to Kotlin a few months ago. Here’s new Matrix33 class for example: https://github.com/JetBrains/skiko/blob/1641d30c27154ce5fda1bdf87e7d2557f61e5a31/skiko/src/commonMain/kotlin/org/jetbrains/skia/Matrix33.kt. I don’t think they have plans to pull from Skija anymore, no (and it will be quite painful, as I can imagine, after the java→kotlin conversion).

No public discussion or announcement as I know of.

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

2 participants