TheTermos / mobkit

Entity API for Minetest
MIT License
26 stars 12 forks source link

Suggestions #15

Closed rubenwardy closed 3 years ago

rubenwardy commented 4 years ago

I'm looking for mob mods to be improved and then merged into Minetest Game

This looks very well done. The main concern I have is that the entire API is in one Lua file, it would be good to have the base API and common behaviours (hq/lq) in different files.

You should consider using luacheck

Some other misc thoughts:

TheTermos commented 4 years ago

I'm looking for mob mods to be improved and then merged into Minetest Game

This looks very well done. The main concern I have is that the entire API is in one Lua file, it would be good to have the base API and common behaviours (hq/lq) in different files.

Totally agreed, I even tried to do it once, but then I realized executing a lua file doesn't preserve enclosing scope and I had to revert it, I'm gonna try again.

You should consider using luacheck

Thanks, checking it out.

  • You don't document the signature of the logic function (ie: what does it take as arguments?)

Checking.

  • Adding methods to the entity def. Rather than doing mobkit.thing(self, ...) it would be nicer to do self:thing(...)

I considered that, ultimately I decided not to bloat entity definitions with all that functions they might not even need. Take behaviors, all of them must take self, they can be arbitrarily many and any entity uses just a small number of them, if any.

TheTermos commented 4 years ago

Just merged one of your PR's, took a while, sorry, I've been busy with that engine thing.

Speaking of which, your name popped up in a related discussion here https://github.com/minetest/minetest/issues/7107 You see, if you won't let me (or someone else) fix the engine, that is going to make further development of this api pointless, and your suggestions irrelevant.

Maybe something more elaborate than a thumbs down emote would be in order?

rubenwardy commented 4 years ago

You see, if you won't let me (or someone else) fix the engine, that is going to make further development of this api pointless, and your suggestions irrelevant.

I don't see how this is relevant. Did you link the wrong issue?

TheTermos commented 4 years ago

No, that's exactly the one. The controversy has the potential to prevent this PR https://github.com/minetest/minetest/pull/9343/ which in turn addresses engine bugs I have found during mobkit development.

You seemed to support the idea of keeping the sneak bug option in both player movement code versions, but an emote alone is quite vague. I'm just curious where you stand on that issue.

rubenwardy commented 4 years ago

There's a whole bunch of history that lead up to that issue. The sneak glitch bug was fixed which lead to a lot of controversy. It was reimplemented as a supported feature in new move, and then the option to use old move readded. My standing is to avoid fragmentation and more arguments

TheTermos commented 4 years ago

I see. I can only hope MT isn't going to willingly choose to stay broken when it comes to that.

rubenwardy commented 4 years ago

You're pretty safe to fix things in new move, the die hards all stick to old move any way

TheTermos commented 4 years ago

That's exactly what I was hoping to hear, thank you.

TheTermos commented 3 years ago

The last point has been done, that is the separation of the core library and example behaviors, so I guess it's good to be closed. Thanks.