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

KGL-Math #19

Open
wants to merge 63 commits into
base: master
Choose a base branch
from
Open

KGL-Math #19

wants to merge 63 commits into from

Conversation

nlbuescher
Copy link
Collaborator

@nlbuescher nlbuescher commented Feb 7, 2020

Kotlinic basic implementation of vector and 4 by 4 matrix types to allow for basic 3d transformation.

  • Vec2 / MutableVec2
  • Vec3 / MutableVec3
  • Vec4 / MutableVec4
  • Mat4 / MutableMat4

kgl-math/src/commonMain/kotlin/com/kgl/math/Vectors.kt Outdated Show resolved Hide resolved
kgl-math/src/commonMain/kotlin/com/kgl/math/Vectors.kt Outdated Show resolved Hide resolved
kgl-math/src/commonMain/kotlin/com/kgl/math/Vectors.kt Outdated Show resolved Hide resolved
kgl-math/src/commonMain/kotlin/com/kgl/math/Vectors.kt Outdated Show resolved Hide resolved
kgl-math/src/commonMain/kotlin/com/kgl/math/Vectors.kt Outdated Show resolved Hide resolved
kgl-math/build.gradle.kts Show resolved Hide resolved
@nlbuescher
Copy link
Collaborator Author

Should there be in-place-modifying variants of abs, clamp, etc for Mutable Vector types? Intentionally creating a function with side-effects like that feels dirty, but it might be important? Perhaps implemented as extensions?

@Dominaezzz
Copy link
Owner

I think there should be. Definitely as extension functions. Naming will be difficult for some of them. Like abs.

@nlbuescher
Copy link
Collaborator Author

The naming is going to be very diifficult, or ugly, depending on whether we insist on nice names, or we use a name like absAssign (🙁). Then, if we have extension functions for mutable types, there should be corresponding extension functions for the immutable types (ie, abs and the like should not be implemented as top-level functions for Vector and Matrix types). Thanks for the input.

@Dominaezzz
Copy link
Owner

Actually, what is abs? Is it component-wise abs? If so, I like the idea of <name>Assign for strictly component-wise operations and <name>ed for otherwise.

@Dominaezzz
Copy link
Owner

Ooh, add quaternions to the list.

@nlbuescher
Copy link
Collaborator Author

abs, ceil, etc are component-wise, yes.

quaternions are part of the recommended extensions (GTC) in GLM. I'll be getting to those. 👍

Copy link
Owner

@Dominaezzz Dominaezzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments.

buildSrc/src/main/kotlin/codegen/math/Utils.kt Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/codegen/math/GenerateMath.kt Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/codegen/math/GenerateMath.kt Outdated Show resolved Hide resolved
buildSrc/src/main/kotlin/codegen/math/GenerateMath.kt Outdated Show resolved Hide resolved
U_BYTE, U_SHORT, U_INT, U_LONG ->
"return sqrt((this dot this).toDouble()).to${type.simpleName.drop(1)}().to${type.simpleName}()"
else ->
"return sqrt(this dot this)"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be sqrt(lengthSquared).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was intending to save a function call (of the lengthSquared getter). Should I not?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I trust the JVM to inline it but I'm not sure about LLVM. We can leave it until we start bench-marking.

@nlbuescher
Copy link
Collaborator Author

Do we need a BooleanVector?

@Dominaezzz
Copy link
Owner

I already think non-floating point vectors are pushing it a bit. So I'm gonna say no.

@nlbuescher
Copy link
Collaborator Author

Works for me. I don't see a use for them either, but could be added later if someone presents a usecase.

@nlbuescher
Copy link
Collaborator Author

nlbuescher commented Mar 2, 2020

Actually, what is abs? Is it component-wise abs? If so, I like the idea of <name>Assign for strictly component-wise operations and <name>ed for otherwise.

So in implementing this a question came up: should abs, ceil, etc be treated like plus, minus, etc (immutable op, mutable opAssign), or like normalize (immutable oped, mutable op)?

Both the arithmetic operators and the normalize function are component-wise.

I'm leaning towards the latter, both because the parameter/return pattern matches normalize more than plus, and because absAssign looks bad.

EDIT: looking at the clamping functions in Kotlin (coerceAtLeast, coerceAtMost, coerceIn), the op/opAssign pattern breaks down, or rather looks even worse.

EDIT2: I take it back. The oped/op pattern is less clear than the op/opAssign pattern IMO after looking at it for a bit.

@Dominaezzz
Copy link
Owner

By component-wise, I mean you can run the operation on each component independently of other components. normalize requires the other components to calculate length.

@nlbuescher
Copy link
Collaborator Author

Got it 👌

x.abs / x.absAssign -> abs(x) / absInPlace(x)

functions that in stdlib take a parameter will follow op / opInPlace, and extension functions will follow op / opAssign
# Conflicts:
#	buildSrc/src/main/kotlin/codegen/math/CoreFeatures.kt
#	buildSrc/src/main/kotlin/codegen/math/ExperimentalExtensions.kt
#	buildSrc/src/main/kotlin/codegen/math/GenerateMath.kt
#	buildSrc/src/main/kotlin/codegen/math/StableExtensions.kt
@nlbuescher
Copy link
Collaborator Author

So I now have basic implementations for the three common vector types and a 4 by 4 matrix, which is enough for view transforms. Still missing are a toMutable... function for all the types, and a corresponding to... function which would replace the copy function that some types currently have.

The tests are in place for all functions. The CI failed because it couldn't download stb in on of the runs, for some reason.

@Dominaezzz
Copy link
Owner

It occasionally happens (Might be worth throwing in some retry logic in there at some point). I've restarted the build, so it should work fine now.

…and expose that storage through asFloatArray function on mutable types
@nlbuescher
Copy link
Collaborator Author

nlbuescher commented May 29, 2020

I've shortened type names to only distinguishing information (EDIT2: actually writing out FloatMatrix4x4.IDENTITY.toMutableFloatMatrix4x4() highlighted the issue with names that are too descriptive). Shortening Vector to Vec and Matrix to Mat are common enough, and types other than Float are not necessary for a basic math implementation. At this point, the API is ready to use, although I still have to figure out what to do with Quaternions. 3D rotation is currently supported via rotation matrices.

EDIT: I'd consider it ready for an alpha/beta release, though

EDIT3: Having a full port of GLM in the new multiplatform version of the kotlin glm project seems like a better focus of efforts, although I think the current implementation here is either a good hold-over, or a good basic implementation if glm is too heavy.

@Dominaezzz
Copy link
Owner

Ah. I'm just seeing this comment now for some reason. It does seem more productive to contribute to the glm project. It will probably end up being a top down rewrite as we shove Kotlin in and pull C out of it.
I'm not able to properly contribute to it right now, my focus on gamedev has drifted elsewhere, but I can review where possible.

I'll probably merge this into a math branch and publish it with a different version suffix. Like 0.1.9-math-alpha-funky-thoughtful-name.

@nlbuescher
Copy link
Collaborator Author

In that case I'm gonna say this PR is no longer WIP. Let me know if you need me to change anything about the PR so it can be pulled into the correct branch.

@nlbuescher nlbuescher changed the title [WIP] KGL-Math KGL-Math Jun 2, 2020
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.

None yet

2 participants