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 |
ctrueden
left a comment
There was a problem hiding this comment.
Here are some thoughts based on my naive reading of the code, as a Kotlin newbie. Maybe some of it helps, I dunno! 🤷 👍
| 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.
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.
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.
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.
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.
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.
How much does it take the publication over there? How long before it's available for consumption?
There was a problem hiding this comment.
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.
| protected fun refineTriangles(recursionLevel: Int, | ||
| vertices: MutableList<Vector3f>, | ||
| indices: MutableList<Triple<Int, Int, Int>>): MutableList<Triple<Int, Int, Int>> { | ||
| indices: MutableList<Face>): MutableList<Face> { |
|
|
||
| // store it, return index | ||
| middlePointIndexCache.put(key, i) | ||
| middlePointIndexCache[key] = i |
There was a problem hiding this comment.
@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.
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.
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.
I'm using absolute methods to upload data, that's why
There was a problem hiding this comment.
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.
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.
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]
| package graphics.scenery.utils | ||
|
|
||
| import kool.Ptr | ||
| import kool.unsafe |
There was a problem hiding this comment.
❗ unsafe? Like sun.misc.Unsafe? Do you discourage its use? Or...?
There was a problem hiding this comment.
Yep, atm it's the way for low level native operations
The whole Lwjgl ultimately rely on that as well
| volumeManager.customTextures.add("OutputRender") | ||
|
|
||
| val outputBuffer = MemoryUtil.memCalloc(1280*720*4) | ||
| val outputBuffer = ByteBuffer(1280*720*4) |
There was a problem hiding this comment.
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.
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.
Nice. As long as they are actually returning ByteBuffer and FloatBuffer objects, that's awesome.
As titled:
Geometry::recalculateNormalsandIcosphereconstructor to pointers