-
Notifications
You must be signed in to change notification settings - Fork 33
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
base: main
Are you sure you want to change the base?
Conversation
- 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
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 |
There was a problem hiding this 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") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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> { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 }
There was a problem hiding this comment.
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.
As titled:
Geometry::recalculateNormals
andIcosphere
constructor to pointers