Quillraven / Fleks

Fast, lightweight, multi-platform entity component system in Kotlin
MIT License
180 stars 20 forks source link

tweak entity modification KDocs #107

Closed aSemy closed 1 year ago

aSemy commented 1 year ago

Hi 👋, just a small suggestion for docs about the entity modification restriction, to make them more clear. This is my subjective suggestion, so feedback would be very welcome!

I thought I'd make a PR directly so it's easier to view my suggested changes, I hope that's okay!

Summary

aSemy commented 1 year ago

Actually, thinking about it, could this conflict be prevented by using a @DslMarker? If there was a @EntityModificationScope DSL marker added to EntityUpdateContext and EntityCreateContext, could that prevent such accidental updates?

Quillraven commented 1 year ago

Not sure if a @DslMarker helps. Can you try it out? I remember that I implemented a safety back then to verify that the entity passed in the cinfugre Lambdas is the correct entity. But this is actually rather complex from what I remember when it comes to nested stuff like:

entity.configure {
  entity.configure {

  }

  entity.configure {
    entity.configure{
    }
  }
}

You'd need like a stack to keep track of it and that had a rather expensive negative impact on performance. That's why we went with the documentation approach.

I like the suggestion but need to have a look how it shows up in the IDE. If you have a good solution to prevent this from happening at all without having a big performance impact, then this is of course also very welcome! :)

Quillraven commented 1 year ago

I have read the documentation about @DslMarker again yesterday and imo it cannot help here. The marker helps that you don't nest DSL functions in an unintended way. But we actually want to achieve the arguments passed to the DSL function are restricted in a special way.

I don't think that this is feasible with a DslMarker. Still, if you have a good idea to solve that without impacting performance to much, please feel free to create a new PR for it.

Thanks for the documentation fixes!