utopia-rise / godot-kotlin-jvm

Godot Kotlin JVM Module
MIT License
576 stars 39 forks source link

Moving buffer.rewind() at the beginning of calls instead of end. #509

Closed CedNaru closed 10 months ago

CedNaru commented 10 months ago

It's something that has been bothering me for a while, debugging the project and going back and forth between the Kotlin and C++. When, for some reason, the data in the shared buffer are not valid, the module is going to throw an error. The error is often things like the number of arguments not matching, or the type of the variable not being the expected one, etc.... The issue with that approach is that those errors are often thrown before rewind() is called, leaving the buffer cursor is an invalid position instead of going back to 0. The consequence of that is that once you have got a first sharedbuffer error, it's going to cascade into all the next calls throwing errors as well because the cursor was never reset and so the data is incorrectly read. It makes the logs full of irrelevant messages, making it harder to decipher.

I propose to change all the TransferContext methods so rewind() is now called at the beginning of each call. It shouldn't alter the regular behavior of the ShareBuffer and would also allow the module to "recover" after an error. The only difference would be that the Sharedbuffer is always left at a non-zero position after being used. But I don't think there should be any issue, as the next method using the buffer should reset it anyway.

It's a simple change, only meant to make the module a bit easier to debug, by removing noises after a first error.

chippmann commented 10 months ago

A much needed improvement indeed and a low hanging fruit. I see no issues with this approach.

piiertho commented 10 months ago

Definitely a good idea. This will help a lot debugging.