minetest-whynot / whynot-game

Minetest game in minetest mods collection style
GNU General Public License v3.0
14 stars 7 forks source link

another fork for helicopter #52

Open bell07 opened 2 years ago

bell07 commented 2 years ago

Consider if https://github.com/ronoaldo/helicopter is better fork


Section added by @Lazerbeak12345 to keep track of when this won't be breaking rule 10

Lazerbeak12345 commented 2 years ago

That repo looks identical to this one: https://github.com/APercy/helicopter (which has more interaction, with 2 PRs and 0 issues, other has none of both)

Lazerbeak12345 commented 2 years ago

The APercy helicopter actually is more balanced - as it requires biofuel to operate.

It also seems higher quality than the present one.

It's on content DB.

Requires biofuel (not currently in whynot. I'll make a request for it)

This mod is registered as nss_helicopter to prevent issues... but I think we will just want to disable helicopter

Handles much more like helicopters from Microsoft Flight sim - so I assume more accurately.

Hitbox is a little small. hard to board.

Crafting recipe makes it more expensive. (The old one was too cheap anyway)

Lazerbeak12345 commented 2 years ago

As for code, they don't need the effectivly empty heli_hud.lua file anymore. Otherwise, I think it's good!

Lazerbeak12345 commented 2 years ago

Added request for dependency biofuel.

Lazerbeak12345 commented 2 years ago

Request

This is the URL for the mod I would like to be added:

https://github.com/APercy/helicopter

My opinion on how it fits with the whynot rules:

Refer to the Whynot Readme for full rule descriptions/reasons.

  1. In what ways might this mod take over the game?
    • None
  2. When could this mod be a strain on the server when no players are using the mod?
    • Never.
  3. When does this mod destroy player's work?
    • Never.
  4. Have you looked at the code? If so, what stood out as things that might need fixing?
    • [ ] Yes. they don't need the effectivly empty heli_hud.lua file anymore
  5. In what way might this mod be reduced to be more simple (as in "Keep it Simple Stupid") (ex: "the foobar mod could be made more simple by splitting into two mods, foo and bar")
    • It's fine as it is.
  6. Is this mod survival freindly? What items that it provides that should be craftable/obtainable, but arent?
    • Yes. None.
  7. When does this mod feel like cheating?
    • Never
  8. Does this mod use the software "git" for version control? (note: we are asking about git. Github, Gitlab, notabug and hundreds of other git providers exist but are not specificly needed, although these do qualify).
    • Yes.
  9. Upon testing this mod, what errors, odd behavior, or other incompatibilities were noticed?
    • None.
    • [ ] Licence issues.

Other comments

Lazerbeak12345 commented 2 years ago

I'm currently looking into what it would take to ensure that we don't break exsisting helis by replacing.

The two mods seem to be capable of working simultaneously - but it should be simple enough to detect that the older mod isn't present and provide aliases or other tricks to ensure that we don't get odd behavior such as unknown nodes.

dacmot commented 2 years ago

Good work. I'm happy to include this mod in whynot to replace the old helicopter with this one, and use biofuel to power it. I feel like it will make a good quest.

Lazerbeak12345 commented 2 years ago

The two mods seem to be capable of working simultaneously - but it should be simple enough to detect that the older mod isn't present and provide aliases or other tricks to ensure that we don't get odd behavior such as unknown nodes.

It looks like this feature has not been implemented yet in Apercy's heli.

I plan on making a PR to add this feature, after researching the best way to make aliases between the entities.

The old heli adds these entities:

The Apercy heli entities:

Lazerbeak12345 commented 2 years ago

I'm having diffuculty understanding how to make this work. Here's the proof-of-concept code I have:

minetest.register_alias("helicopter:heli", "nss_helicopter:heli")

In a test with a bunch of old helis in the world, it doesn't work and logs that there are a lot of "helicopter:heli" that It doesn't know what are.

dacmot commented 2 years ago

I have to admit, I'm not very familiar with using aliases (yet).

dacmot commented 2 years ago

Based on the description in lua_api.txt, I think you might have them reversed.

Lazerbeak12345 commented 2 years ago

I've now tried both. I can't seem to figure it out. My assumption is that I need to prefix one of them with : in the string or that I'll just have to register a new entity that drops helicopter parts or wood or something. The latter might be better, since it will make up for the important differences between the two helicopters.

Lazerbeak12345 commented 2 years ago

Update:

Since I'm pretty busy right now, I can't test much at once. See this as a devlog of sorts will I get this worked out.

I just tried both combinations, but with a : before the foreign node name. No dice. I aditidly havnen't checked the minetest.log. I plan on checking this next.

Lazerbeak12345 commented 2 years ago

I just pushed heli as-is on their master branch. I don't think we can safely upgrade without solving the alias problem in some way or another.

I'll get back to this issue later. For whatever reason this stressed me out.... lol.

dacmot commented 2 years ago

No worries. There's no rush either ;)

Lazerbeak12345 commented 1 year ago

Pavel's fork (the original, actually) no longer would break rule 10. We can now assess if that fork works in modern MineTest and add it. If downstream follows suit, we should at least cross-compare.

Lazerbeak12345 commented 1 year ago

Request

This is the URL for the mod I would like to be added:

https://github.com/SokolovPavel/helicopter

My opinion on how it fits with the whynot rules:

Refer to the Whynot Readme for full rule descriptions/reasons.

  1. In what ways might this mod take over the game?
    • None
  2. When could this mod be a strain on the server when no players are using the mod?
    • Never.
  3. When does this mod destroy player's work?
    • Never.
  4. Have you looked at the code? If so, what stood out as things that might need fixing?
    • [ ] There's a fair bit of code that looks a bit janky
  5. In what way might this mod be reduced to be more simple (as in "Keep it Simple Stupid") (ex: "the foobar mod could be made more simple by splitting into two mods, foo and bar")
    • It's fine as it is.
  6. Is this mod survival freindly? What items that it provides that should be craftable/obtainable, but arent?
    • Yes. Nothing involving crafting is a problem.
    • It'd be nice if it were more expensive.
  7. When does this mod feel like cheating?
    • It'd be nice if it were more expensive.
  8. Does this mod use the software "git" for version control? (note: we are asking about git. Github, Gitlab, notabug and hundreds of other git providers exist but are not specificly needed, although these do qualify).
    • Yes.
  9. Upon testing this mod, what errors, odd behavior, or other incompatibilities were noticed?
    • GPL_v2 or any later version

Other comments

We had been using a fork of this mod, and downstream has features I like a bit better (biofuel).

dacmot commented 1 year ago

Just wondering, why the request for Pavel's helicopter mod? We removed it because of license issues, but we were thinking of replacing it anyway. Why not just replace it immediately?

Lazerbeak12345 commented 1 year ago

Well, if downstream does upgrade their license, then I'd request that version. Downstream is all-around less buggy - which alone might be grounds for rejecting this version.

If we do add a downstream version I'm still concerned about backwards compatibility. After reading through this version again, I think I better understand how to do that.

Lazerbeak12345 commented 3 days ago

We should now be able to use Desour's helicopter.