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

Small kool showcase examples #532

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Small kool showcase examples #532

wants to merge 3 commits into from

Conversation

elect86
Copy link
Member

@elect86 elect86 commented May 26, 2023

As titled:

  • added kool library dependency
  • switched code in Geometry::recalculateNormals and Icosphere constructor to pointers
  • added some pointers and typealias utils
  • using kool constructors and methods to allocate buffers

elect86 added 3 commits May 26, 2023 21:05
- switched code in `Geometry::recalculateNormals` and `Icosphere` constructor to pointers
- added some pointers and typealias utils
- using kool constructors and methods to allocate buffers
… the still missing bitwise operators in Kotlin), fixing with parenthesis
@elect86
Copy link
Member Author

elect86 commented May 30, 2023

Also, it's important to think about when to run the buffers deallocation, otherwise Scenery leaks

I can investigate also in this direction, if you want

Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

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

Here are some thoughts based on my naive reading of the code, as a Kotlin newbie. Maybe some of it helps, I dunno! 🤷 👍

@@ -22,6 +22,7 @@ repositories {
mavenCentral()
maven("https://maven.scijava.org/content/groups/public")
maven("https://jitpack.io")
maven("https://raw.githubusercontent.com/kotlin-graphics/mary/master")
Copy link
Member

Choose a reason for hiding this comment

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

Can we please get artifacts deployed to maven.scijava.org, instead of a custom GitHub-based repository, which should not be done?

Copy link
Member Author

Choose a reason for hiding this comment

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

What shouldn't be done is loading big jars (ie fat jars)

This is not the case, the ones I push weight few KBs

Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be done at all IMHO—now you have to run that repo forever or else old builds will break. And as described in that article, it creates a situation where there's another repository that has to be queried for all the dependencies, slowing down builds. Just release the projects on maven.scijava.org or oss.sonatype.org! Once you have the infrastructure in place for Sonatype, deploying is a button push—the only huge PITA is getting it set up at the beginning.

Copy link
Member Author

Choose a reason for hiding this comment

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

now you have to run that repo forever or else old builds will break.

True, but you know what I have to do to run it forever? Just avoid deleting it

I might be move everything to maven, though (here)

I'd love to deploy to maven.scijava.org, but I have a dev way to release very often and that wasn't compatible with that, or do I recall it wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Publishing to maven.scijava.org is just a standard deploy over WebDAV, I'm sure the Gradle tooling will support it with your credentials both manually and/or automatically. As long as your tooling doesn't try to deploy the same release artifact more than once, which is forbidden due to release artifacts being immutable.

Copy link
Member Author

Choose a reason for hiding this comment

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

How much does it take the publication over there? How long before it's available for consumption?

Copy link
Member

Choose a reason for hiding this comment

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

Artifacts published to maven.scijava.org are available instantly. Artifacts published to oss.sonatype.org (or s01.oss.sonatype.org) are available instantly from those URLs, but typically take 10-30 minutes to get mirrored over to Maven Central.

@@ -73,11 +82,11 @@ open class Icosphere(val radius: Float, val subdivisions: Int) : Mesh("Icosphere

protected fun refineTriangles(recursionLevel: Int,
vertices: MutableList<Vector3f>,
indices: MutableList<Triple<Int, Int, Int>>): MutableList<Triple<Int, Int, Int>> {
indices: MutableList<Face>): MutableList<Face> {
Copy link
Member

Choose a reason for hiding this comment

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

Hooray for type aliasing! 🏆

@@ -124,7 +133,7 @@ open class Icosphere(val radius: Float, val subdivisions: Int) : Mesh("Icosphere
val i = this.addVertex(middle)

// store it, return index
middlePointIndexCache.put(key, i)
middlePointIndexCache[key] = i
Copy link
Member

Choose a reason for hiding this comment

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

@elect86 Do you notice these manually, or lean on an IDE cleanup/simplification action? If so, what is it?

Copy link
Member Author

@elect86 elect86 Jun 14, 2023

Choose a reason for hiding this comment

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

If suggested, "Alt+Enter" opens the action dialog and then you select the suggestion

Otherwise you need to explicitely call .set in order to force the IDE to pull in the correct import and then execute the suggestion


vertices.flip()
normals.flip()
texcoords.flip()
Copy link
Member

Choose a reason for hiding this comment

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

Just to be sure: the kool FloatBuffer does not require flipping, whereas the one allocated by BufferUtils does?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using absolute methods to upload data, that's why

Copy link
Member

Choose a reason for hiding this comment

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

Cool, I just wanted to make sure these lines didn't get dropped by accident. 👍

for (i in vertices.indices step 3) {
val v1 = pVtx++[0]
val v2 = pVtx++[0]
val v3 = pVtx++[0]
Copy link
Member

Choose a reason for hiding this comment

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

This is super concise, but I find it harder to understand. Maybe a middle ground could be:

fun makeVector(array, i) { return Vector3f(array[i], array[i + 1], array[i + 2]) }

val v1 = makeVector(vertexBufferView, i += 3)
val v2 = makeVector(vertexBufferView, i += 3)
val v3 = makeVector(vertexBufferView, i += 3)

or some such? Or even attach a toVector(i) method to the FloatBuffer class here?

It is fun to use overloaded operators, but the pointer thing with [0] is strange.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's essentially *pVtx++

Another alternative:

        for (i in vertices.indices step 3) {
            val v1 = pVtx[i]
            val v2 = pVtx[i + 1]
            val v3 = pVtx[i + 2]

Copy link
Member

Choose a reason for hiding this comment

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

Nice, I like that much more!

package graphics.scenery.utils

import kool.Ptr
import kool.unsafe
Copy link
Member

Choose a reason for hiding this comment

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

❗ unsafe? Like sun.misc.Unsafe? Do you discourage its use? Or...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, atm it's the way for low level native operations

The whole Lwjgl ultimately rely on that as well

@@ -40,7 +41,7 @@ class CustomVolumeManagerExample : SceneryBase("CustomVolumeManagerExample") {
))
volumeManager.customTextures.add("OutputRender")

val outputBuffer = MemoryUtil.memCalloc(1280*720*4)
val outputBuffer = ByteBuffer(1280*720*4)
Copy link
Member

Choose a reason for hiding this comment

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

I love the conciseness of this. But it makes me nervous that the kool buffer types are named ByteBuffer and FloatBuffer, when java.nio also has classes with those names. Does the possibility of name clashes or reader confusion concern you? If so, you could rename the kool buffer types to something else like KoolByteBuffer and KoolFloatBuffer, or Uint8Buffer and Float32Buffer.

Copy link
Member Author

@elect86 elect86 Jun 14, 2023

Choose a reason for hiding this comment

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

They look like types, but are methods instead. It's a technique called fake/dummy constructors.

You'll recognize those in the IDE by being in italics

It's coherent with the current way to allocate Arrays and Lists

val ints = IntArray(2)
val ints = List(2) { i -> i + 1 }

Copy link
Member

Choose a reason for hiding this comment

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

Nice. As long as they are actually returning ByteBuffer and FloatBuffer objects, that's awesome.

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.

2 participants