BlockManager returns Air block instead of null for block family if no side is defined

[oniatus]

To test, inject a BlockManager in a test system, activate the core module and inspect blockManager.getBlock("core:BirchTrunk:engine:stair"). This call is missing the side parameter for the family rotation, e.g. blockManager.getBlock("core:BirchTrunk:engine:stair.LEFT") gives a correct rotated stair. But instead of logging an error or returning null, this silently returns an Air block 😒 .

Tow classes on issue here: HorizontalBlockFamily.getBlockFor(BlockUri) does not log an warning when the side can not be parsed, returning null may be okay.

BlockManager.getBlock(BlockUri) returns null if the block can not be found directly but an air block if the block is not directly registered, part of a family and the family returns null for the block. This should really return null as the javadoc says: Retrieves the Block for the given uri, or null if there isn't one.

[skaldarnar]

Following up on my question in Slack - are we going for null interfaces or do we promote the use of Optional? Sounds like this would fit here as well, and it might simplify the error handling. Does anybody know of any good resource investigating the impact of Optionals?

[OvermindDL1]

Following up on my question in Slack - are we going for null interfaces or do we promote the use of Optional?

Optional's do have overhead in benchmarks on JVM8, but it is fairly minor. But I do like the expressiveness of them personally.

/me is starting to lean toward thinking kotlin should start to be integrated, you can decorate such returns and arguments 'for free' (zero runtime cost) while being fully backwards compatible with java...

[skaldarnar]

/me holds up the Scala flag 🎏

[OvermindDL1]

/me holds up the Scala flag 🎏

Hehe, I too am a proponent of Scala, however, much to my dismay, Scala's stdlib is HUGE in comparison, and Kotlin has, sadly, become mainstream in the java world, especially now that it is first class on android. ^.^;

If the scala stdlib is contained in terasology (I've yet to check, bleh), I'd be all for using it where appropriate as you can mix it and java (and kotlin) all as you wish.

However, Kotlin's 'possibly-null' operator (?) is zero-cost, unlike boxing that Scala's Option and Java's Optional does, which is definitely valuable, especially in highly called code.

[skaldarnar]

Getting it into engine might need some work to convince people. For modules, however, we already have

• https://github.com/Terasology/ScalaLib
• https://github.com/Terasology/KotlinLib

They both may need some more love and attention, though.

[syntaxi]

Following up to say have we decided between null and Optional yet? @skaldarnar @OvermindDL1

[oniatus]

I would suggest to break the API for v2 or v3 and switch to Optional. It makes the API more explicit and reduces the chance for failures.

[TheFlash98]

Does this issue require all the getBlockFor(blockUri) methods to return Optional instead of a block?

[syntaxi]

I believe that @oniatus is saying it should return null but in V2 should be an Optional So for now BlockManager.getBlock(BlockUri) should return Null (as per javadoc) along with HorizontalBlockFamily.getBlockFor(BlockUri)

[TheFlash98]

As far as I can understand, HorizontalBlockFamily.getBlockFor(BlockUri) does return a null if there's any exception.

[OvermindDL1]

For note, doesn't Optional incur a runtime speed overhead that null (and consequently Kotlin's ?) does not have? If Kotlin is ever expected to be merged in then having the cheaper option is more efficient and still safe in the Kotlin world at least (though less so in the Java/Scala world).

[syntaxi]

Kotlin probably isn't ever going to be merged into the engine, because we already have a module for it. https://github.com/Terasology/KotlinLib

[OvermindDL1]

Even ignoring it though, Optional does still have a runtime overhead that null does not have.

[oniatus]

It is one extra object instance per call. Block lookups are not needed on every frame and the result can be cached so we are in an area where performance is not that critical. My main priority here is expressiveness and accessibility of the api for a new developer.

[Cervator]

Are we sure Kotlin/Scala should remain away from the engine? Not every extended JVM language should be hooked up, surely, can leave the extras for module land. But if there is enough value to hooking in Kotlin and/or (probably or?) Scala to the engine as a permanent thing I'm not sure if that's a big problem if we do it as part of a major release sometime?

I assume the code will work the same for end-users. It'll just be up to us to clarify how such a language would be used in the engine and if there are any additional hoops a developer would need to jump though. Hopefully not? At least in Gradle and probably also IntelliJ?

[OvermindDL1]

I personally really like Scala, it does its typing system Right, but it is not really 'first-class' in that it is not native on anything yet, has to be separate. Kotlin is more of a very simplified Scala, lacks the nice typing features of Scala so you cannot generate, say, super-optimized code (like primitive containers and such), but it is more of a Java Done Right, but the big point of it is that it is First-Class on Android, and if it is first class there (the largest JVM ecosystem) and the largest Java IDE (IntelliJ) includes Kotlin by default for the desktop, it is likely to say that it has become pretty ubiquitous (didn't Kotlin even take over Java in the last few months as the most used JVM language on android?). Scala's code generation is absolutely top notch though... hard to ignore that...

But yeah, Scala and Kotlin and Java can all interoperate fine in all ways without cost (unlike groovy and such).

There shouldn't be any hoops to jump through if Scala/Kotlin is added to the base engine's gradle file, it will set up the scala/kotlin for whatever IDE they use anyway.

[syntaxi]

So I'm going to open a new issue for discussion on Scala/Kotlin .

In the mean time we have v2 on it's way up, should we break API and return an Optional or just use a Null?
@oniatus @OvermindDL1 @TheFlash98

[OvermindDL1]

Null is more efficient and ties with Kotlin's safety better, efficiency is good in an engine core, so that would be my vote.

[vampcat]

I vote for Optional.

If the goal is to make the most efficient code we possibly can, we should switch to C++ (or assembly). However, the goal is to make a easy-to-approach, extensible platform. Which is why I vote in favour of cleaner code, even if it means slight performance hit. (Note that gestalt uses Optionals almost exclusively, so any and every cost associated with Optionals is already being borne by us.)

[Cervator]

Good point on the potential over-optimization. Maybe the Optional vs. Kotlin + null is something to revisit around engine v3.0.0 - at a point where hopefully it'll be time to optimize, with lots of new benchmarking and profiling tools hooked up and tried some. Maybe by then we'll be able to properly evaluate its value.

Not to say that the consideration for Kotlin or Scala is dead simply because one facet related to optimization gets written off for the time being. But anyway :-)

[Cervator]

Slight bump to this: As part of finalizing v2 I'm pushing this out to v3 - although really the fundamental issue here could probably be quickly addressed in a minor or patch release at some point. But it grew into a bigger issue as visible in the comments here, getting into how to handle the return (null vs Optional) rather than just changing from Air to null real quick.

The block family overhaul (PR #3343 / #3149) doesn't change the root issue here (Air/null) although it did rename HorizontalBlockFamily to HorizontalFamily along with an assortment of other tweaks. That PR should be merged soon and released with v2, with another planned round of related refactoring in the v3 release later.

That additional pass I suspect may render the Air/null part a moot point, while the general issue of null vs Optional still needs to be addressed, and may be as part of other efforts during that release, especially if we end up re-integrating gestalt-entity-system which will certainly lead to a bunch of cleanup. I'm referencing all that separately in a roadmap/backlog thing I"m working on and will link to here.