Fix chest boats #2810

MrRar · 2022-10-17T19:57:56+02:00

MrRar commented

2022-10-17 19:57:56 +02:00

Changes:

Player no longer sits in the chest in the chest boat
Chest boats use the same texures and model as regular boats
Oars are no longer backwards and rowing animation is better
Chest boat has no holes or cracks in it.

I will upload the .blend file to the VoxelGoodEnough repo when this PR is merged.

~~Stuff to do:

Fix compatibility with existing boat entites (textures).
Boat inventory should only work for chest boats.

Hopefully this PR will block the imminent MCL2 release or I will need to preserve compatibility with chest boat entities.~~

Changes: - Player no longer sits in the chest in the chest boat - Chest boats use the same texures and model as regular boats - Oars are no longer backwards and rowing animation is better - Chest boat has no holes or cracks in it. I will upload the .blend file to the VoxelGoodEnough repo when this PR is merged. ~~Stuff to do: - Fix compatibility with existing boat entites (textures). - Boat inventory should only work for chest boats. Hopefully this PR will block the imminent MCL2 release or I will need to preserve compatibility with chest boat entities.~~

👍 3

cora commented

2022-10-17 20:23:48 +02:00

Changes:

Player no longer sits in the chest in the chest boat

I just fixed this in #2788

Chest boats use the same texures and model as regular boats

That's nice ... i guess they should not be the same entity though - i think that is a bad idea.

Oars are no longer backwards and rowing animation is better

Chest boat has no holes or cracks in it.

nice

Stuff to do:

Fix compatibility with existing boat entites (textures).

Boat inventory should only work for chest boats.

Currently they do only work for chest boats - just keep them as a separate entity id.

Hopefully this PR will block the imminent MCL2 release or I will need to preserve compatibility with chest boat entities.

Not indefinitely - but if it's just a few days sure.

But as said above I think making them the same entity is a really bad idea (and ig thats why it would be incompatible!?) - to even make that work you'd have to bend backwards p hard and for what exactly ?

> Changes: > - Player no longer sits in the chest in the chest boat I just fixed this in #2788 > - Chest boats use the same texures and model as regular boats That's nice ... i guess they should not be the same entity though - i think that is a bad idea. > - Oars are no longer backwards and rowing animation is better > - Chest boat has no holes or cracks in it. nice > Stuff to do: > - Fix compatibility with existing boat entites (textures). > - Boat inventory should only work for chest boats. Currently they do only work for chest boats - just keep them as a separate entity id. > Hopefully this PR will block the imminent MCL2 release or I will need to preserve compatibility with chest boat entities. Not indefinitely - but if it's just a few days sure. But as said above I think making them the same entity is a really bad idea (and ig thats why it would be incompatible!?) - to even make that work you'd have to bend backwards p hard and for what exactly ?

cora commented

2022-10-17 20:26:57 +02:00

There is no "turn boat into chest boat" mechanic for boats (and minecarts) like there is for the chest-mobs which need a fair bit more of additional code to make it work as you maybe saw.

For minecarts and boats i see absolutely no reason to have them be the same entity (and i mean .. if you want to do it for boats why don't you want to do it for minecarts ? )

I don't get it

There is no "turn boat into chest boat" mechanic for boats (and minecarts) like there is for the chest-mobs which need a fair bit more of additional code to make it work as you maybe saw. For minecarts and boats i see absolutely no reason to have them be the same entity (and i mean .. if you want to do it for boats why don't you want to do it for minecarts ? ) I don't get it

cora commented

2022-10-17 21:56:31 +02:00

plus it should be pointed out that there are servers running on master out there. While we obviously cant guarantee anything with that we do not need to break their worlds for no reason other than that it is possible to do that if the boat models are the same !?

plus it should be pointed out that there are servers running on master out there. While we obviously cant guarantee anything with that we do not need to break their worlds for no reason other than that it is *possible* to do that if the boat models are the same !?

cora commented

2022-10-17 22:07:45 +02:00

I mean the incompatibility could easily be fixed by adding the entity twice with both names ... but as i said it only leads to more complex code in mcl_boats .. don't really see a reason to do it at all

cora commented

2022-10-17 22:21:49 +02:00

The model looks very nice though

❤️ 1

MrRar commented

2022-10-17 22:39:38 +02:00

For minecarts and boats i see absolutely no reason to have them be the same entity

My thought is that since boats and chest boats are sharing 90% of the code you might as well make them the same entity for simplicity's sake. Imagine if you registed every type of villager as a different entity.

However I don't have a very strong opinion on that, so i'm just going to do it they way you like it. So disregard my statement about blocking the MCL2 release.

> For minecarts and boats i see absolutely no reason to have them be the same entity My thought is that since boats and chest boats are sharing 90% of the code you might as well make them the same entity for simplicity's sake. Imagine if you registed every type of villager as a different entity. However I don't have a very strong opinion on that, so i'm just going to do it they way you like it. So disregard my statement about blocking the MCL2 release.

❤️ 1

cora commented

2022-10-17 23:02:52 +02:00

However I don't have a very strong opinion on that, so i'm just going to do it they way you like it. So disregard my statement about blocking the MCL2 release.

I guess I do ... well there was a reason i did it this way - because doing it the other way would require a fair bit more code (as you can see in chest-mobs vs. minecarts - for the latter it's literally just the register_entity_inv) - that's essentially the simple case of an entity inv: an entity that always has an inv of the same size.

Now I do plan on abstracting the additional stuff that's in the mobs to entity_invs as well - at least the part that's the same: chest gets added, it gets an inv of a size set by an entity field, the on_rightclick to open the formspec gets added. But I have not done that yet so all of that you would have to do here too if you did it this way - essentially ditching a good part of what the api can do to reimplement it here ^^

> However I don't have a very strong opinion on that, so i'm just going to do it they way you like it. So disregard my statement about blocking the MCL2 release. I guess I do ... well there was a reason i did it this way - because doing it the other way would require a fair bit more code (as you can see in chest-mobs vs. minecarts - for the latter it's literally just the register_entity_inv) - that's essentially the simple case of an entity inv: an entity that always has an inv of the same size. Now I do plan on abstracting the additional stuff that's in the mobs to entity_invs as well - at least the part that's the same: chest gets added, it gets an inv of a size set by an entity field, the on_rightclick to open the formspec gets added. But I have not done that yet so all of that you would have to do here too if you did it this way - essentially ditching a good part of what the api can do to reimplement it here ^^

cora commented

2022-10-17 23:15:04 +02:00

So yeah deadline is end of the week i guess - got the weekend off so i can do some serious testing.

I wanted to realease last week tbh. but then @epCode's mob fixes came in and they were so nice that i just could not help but merge them. But not really surprisingly they need some more testing – mobs have just such a large "surface area".

Maybe if we stay this active we need to make release branches and shit. We'll see - at this point it's still manageable i guess.

It would be nice to have those model fixes in the release though - and i think your sitting position is better too if I'm honest.

So yeah deadline is end of the week i guess - got the weekend off so i can do some serious testing. I wanted to realease last week tbh. but then @epCode's mob fixes came in and they were so nice that i just could not help but merge them. But not really surprisingly they need some more testing – mobs have just such a large "surface area". Maybe if we stay this active we need to make release branches and shit. We'll see - at this point it's still manageable i guess. It would be nice to have those model fixes in the release though - and i think your sitting position is better too if I'm honest.

MrRar force-pushed chest_boats from 590240bf96 to cb2c01b1e2

2022-10-18 17:04:47 +02:00

Compare

MrRar changed title from ~~WIP: Fix chest boats~~ to Fix chest boats

2022-10-18 17:07:28 +02:00

cora approved these changes 2022-10-19 12:58:03 +02:00

cora left a comment

works

cora merged commit e56167136b into master

2022-10-19 12:58:15 +02:00

cora referenced this issue from a commit

2022-10-19 12:58:17 +02:00