ITEMS/mcl_chests: Fix Ender chests from MineClone2 #103

erlehmann · 2021-06-28T22:53:57+02:00

erlehmann commented

2021-06-28 22:53:57 +02:00

Problem

TRACKING ISSUE: #81

Commit 819dbc6224 of MineClone2 introduced
an LBM that removed Ender chest formspecs stored in the node meta. That
change makes Ender chests that were loaded in MineClone2 versions past
that commit not show the Ender chest inventory form on right-click.

Solution

This patch makes those broken Ender chests work by introducing an LBM
that writes the formspec to the node meta for Ender chest nodes once.

Details

Since the LBM name is suffixed with a hash of the Ender chest formspec,
changes to the Ender chest formspec (even removing it entirely) should
not require manual adjustment of the LBM code.

Testing Steps

Create a new world in MineClone2 0.71.0 (commit e9182a9506).
Execute the following commands in the in-game chat/console:

/grantme give
/giveme mcl_chests:ender_chest 99

Verify that you have 99 Ender chests in your inventory.
Place an Ender chest.
Put an Ender chest in the Ender chest.
Verify that you have 97 Ender chests in your inventory.
Open the world with MineClone2 commit 04436ea5f765edc26529122f3586b59058604f80.
Place an Ender chest.
Put an Ender chest in the Ender chest.
Verify that you have 95 Ender chests in your inventory.
Open the world in Mineclonia commit 926d5e2c37.
Verify that you can not access the Ender chest inventory via right clicking the Ender chest placed in step 4.
Verify that you can not access the Ender chest inventory via right clicking the Ender chest placed in step 8.
Place an Ender chest.
Put an Ender chest in the Ender chest.
Verify that you have 93 Ender chests in your inventory.
Open the world in Mineclonia commit f8c58262bc.
Right click each of the Ender chests placed in steps 4, 8, 14 and take a single Ender chest from each inventory.
Verify that each Ender chests closes when you close the formspec.
Verify that you have 96 Ender chests in your inventory.

To do

Fill out PR template
Create testing steps

##### Problem TRACKING ISSUE: https://git.minetest.land/Mineclonia/Mineclonia/issues/81 Commit https://git.minetest.land/MineClone2/MineClone2/commit/819dbc6224c3b96ad4094cccf3d9150f3ef61d45 of MineClone2 introduced an LBM that removed Ender chest formspecs stored in the node meta. That change makes Ender chests that were loaded in MineClone2 versions past that commit not show the Ender chest inventory form on right-click. ##### Solution This patch makes those broken Ender chests work by introducing an LBM that writes the formspec to the node meta for Ender chest nodes once. ##### Details Since the LBM name is suffixed with a hash of the Ender chest formspec, changes to the Ender chest formspec (even removing it entirely) should not require manual adjustment of the LBM code. ##### Testing Steps 1. Create a new world in MineClone2 0.71.0 (commit e9182a9506b66c536ce50d6df086e4758b47c0c2). 2. Execute the following commands in the in-game chat/console: - `/grantme give` - `/giveme mcl_chests:ender_chest 99` 3. Verify that you have 99 Ender chests in your inventory. 4. Place an Ender chest. 5. Put an Ender chest in the Ender chest. 6. Verify that you have 97 Ender chests in your inventory. 7. Open the world with MineClone2 commit 04436ea5f765edc26529122f3586b59058604f80. 8. Place an Ender chest. 0. Put an Ender chest in the Ender chest. 10. Verify that you have 95 Ender chests in your inventory. 11. Open the world in Mineclonia commit https://git.minetest.land/Mineclonia/Mineclonia/commit/926d5e2c37359659b7406291d03ec720d2ab7f81. 12. Verify that you can not access the Ender chest inventory via right clicking the Ender chest placed in step 4. 13. Verify that you can not access the Ender chest inventory via right clicking the Ender chest placed in step 8. 14. Place an Ender chest. 15. Put an Ender chest in the Ender chest. 16. Verify that you have 93 Ender chests in your inventory. 17. Open the world in Mineclonia commit https://git.minetest.land/Mineclonia/Mineclonia/commit/f8c58262bc024659eb3ba53dbc006a917054ba7f. 18. Right click each of the Ender chests placed in steps 4, 8, 14 and take a single Ender chest from each inventory. 19. Verify that each Ender chests closes when you close the formspec. 20. Verify that you have 96 Ender chests in your inventory. ##### To do - [x] Fill out PR template - [x] Create testing steps

erlehmann force-pushed fix-ender-chests-from-mineclone2 from 257427ec7a to f8c58262bc

2021-06-28 22:55:15 +02:00

Compare

erlehmann referenced this pull request

2021-06-29 08:42:15 +02:00

Ender chests on Clamity Anarchy server do not show inventory form #81

ryvnf reviewed 2021-06-29 12:41:19 +02:00

mods/ITEMS/mcl_chests/init.lua

					
				@ -1404,0 +1408,4 @@

				minetest.register_lbm({

					label = "Update ender chest formspecs (MineClone2 compatibility)",

					name = "mcl_chests:update_ender_chest_formspecs_" ..

						minetest.sha1(formspec_ender_chest),

ryvnf commented

2021-06-29 12:41:19 +02:00

For some reason I have missed that there exists a sha1 hash function. I've used get_password_hash for things like this in the past. :)

Anyways. What is the purpose of including the hash in a formspec name in this case?

For some reason I have missed that there exists a `sha1` hash function. I've used `get_password_hash` for things like this in the past. :) Anyways. What is the purpose of including the hash in a formspec name in this case?

erlehmann commented

2021-06-29 19:55:19 +02:00

To quote myself:

Since the LBM name is suffixed with a hash of the Ender chest formspec,
changes to the Ender chest formspec (even removing it entirely) should
not require manual adjustment of the LBM code.

What this means in practice is that by using the hash of the formspec, this code will handle all future formspecs too: Any change of the formspec changes the hash, which creates a new LBM to update the formspec. The only limitation is that you can not go back to a formspec that has the same SHA1 hash as a previous formspec that way and expect it to work.

To quote myself: > Since the LBM name is suffixed with a hash of the Ender chest formspec, changes to the Ender chest formspec (even removing it entirely) should not require manual adjustment of the LBM code. What this means in practice is that by using the hash of the formspec, this code will handle all *future* formspecs too: Any change of the formspec changes the hash, which creates a new LBM to update the formspec. The only limitation is that you can not go back to a formspec that has the same SHA1 hash as a previous formspec that way and expect it to work.

erlehmann marked this conversation as resolved

ryvnf commented

2021-06-29 13:05:34 +02:00

In general, I am a bit skeptical towards changes like this. I also think it is important that we do not let this project become too coupled with the Clamity Anarchy server, by making changes specifically to make it work better. I think it is good that this change is described (in commit message and the comment) in terms of its general use-case and not specifically for Clamity Anarchy.

I do however think that users can always expect things breaking by going from latest MineClone2 to Mineclonia because we have no control over the changes they make. This is probably far from the only thing which breaks. This begs the question if Mineclonia should add code like this for forward compatibility. I think it is reasonable to expect MineClone2 and Mineclonia to be incompatible.

I am not going to reject the PR, since removing this LBM later would be a simple change. The performance impact of this should also be negligible. I will let somebody else decide if this should be merged.

In general, I am a bit skeptical towards changes like this. I also think it is important that we do not let this project become too coupled with the Clamity Anarchy server, by making changes specifically to make it work better. I think it is good that this change is described (in commit message and the comment) in terms of its general use-case and not specifically for Clamity Anarchy. I do however think that users can always expect things breaking by going from latest MineClone2 to Mineclonia because we have no control over the changes they make. This is probably far from the only thing which breaks. This begs the question if Mineclonia should add code like this for forward compatibility. I think it is reasonable to expect MineClone2 and Mineclonia to be incompatible. I am not going to reject the PR, since removing this LBM later would be a simple change. The performance impact of this should also be negligible. I will let somebody else decide if this should be merged.

erlehmann commented

2021-06-29 20:07:25 +02:00

In general, I am a bit skeptical towards changes like this. I also think it is important that we do not let this project become too coupled with the Clamity Anarchy server, by making changes specifically to make it work better. I think it is good that this change is described (in commit message and the comment) in terms of its general use-case and not specifically for Clamity Anarchy.

Yes, everyone who wants to switch from recent MineClone2 to Mineclonia will benefit from this patch.

I do however think that users can always expect things breaking by going from latest MineClone2 to Mineclonia because we have no control over the changes they make. This is probably far from the only thing which breaks. This begs the question if Mineclonia should add code like this for forward compatibility. I think it is reasonable to expect MineClone2 and Mineclonia to be incompatible.

I think we should not ever make any guarantee of full compatibility, because, as you pointed out, we have no control about what MineClone2 does. We can try to make any switch as easy as possible though.

I am not going to reject the PR, since removing this LBM later would be a simple change.

This LBM should probably not ever be removed, as it handles three different purposes:

The LBM ensures MineClone2 compatibility for Ender Chests (for now).
The LBM ensures that any future changes to the Ender chest formspec are propagated.
In case Mineclonia removes formspecs from node meta, the LBM can be used to reset the node meta.

I think this is probably the most future-proof code I have written in a while.

The performance impact of this should also be negligible.

I have no idea. Do you have any documents on the performance impacts of LBMs?

> In general, I am a bit skeptical towards changes like this. I also think it is important that we do not let this project become too coupled with the Clamity Anarchy server, by making changes specifically to make it work better. I think it is good that this change is described (in commit message and the comment) in terms of its general use-case and not specifically for Clamity Anarchy. Yes, everyone who wants to switch from recent MineClone2 to Mineclonia will benefit from this patch. > I do however think that users can always expect things breaking by going from latest MineClone2 to Mineclonia because we have no control over the changes they make. This is probably far from the only thing which breaks. This begs the question if Mineclonia should add code like this for forward compatibility. I think it is reasonable to expect MineClone2 and Mineclonia to be incompatible. I think we should not ever make any guarantee of full compatibility, because, as you pointed out, we have no control about what MineClone2 does. We can try to make any switch as easy as possible though. > I am not going to reject the PR, since removing this LBM later would be a simple change. This LBM should probably not ever be removed, as it handles three different purposes: 1. The LBM ensures MineClone2 compatibility for Ender Chests (for now). 2. The LBM ensures that any future changes to the Ender chest formspec are propagated. 3. In case Mineclonia removes formspecs from node meta, the LBM can be used to reset the node meta. I think this is probably the most future-proof code I have written in a while. > The performance impact of this should also be negligible. I have no idea. Do you have any documents on the performance impacts of LBMs?

erlehmann changed title from ~~WIP: ITEMS/mcl_chests: Fix Ender chests from MineClone2~~ to ITEMS/mcl_chests: Fix Ender chests from MineClone2

2021-06-29 22:12:46 +02:00