node-minecraft-data icon indicating copy to clipboard operation
node-minecraft-data copied to clipboard

add getRenamedData

Open zardoy opened this issue 1 year ago • 20 comments

ref https://github.com/PrismarineJS/minecraft-data/pull/836, really want to have it!

zardoy avatar Jan 18 '24 14:01 zardoy

diamond-square and flying-squid will definitely benefit from it: https://github.com/PrismarineJS/diamond-square/pull/15/

(so we don't have to open another pr in future for the short_grass rename)

zardoy avatar Jan 21 '24 02:01 zardoy

Why would that benefit? These renames should be done via features instead. We don't just support one version or the other but all new versions past a minimum one

extremeheat avatar Jan 21 '24 03:01 extremeheat

Why would that benefit?

Because I can use simple function like getMcData.getRenamedData('blocks', 'tall_grass', '1.17.1', version). And that's it. I do not need to care of anything else. With features I need to keep in mind what blocks were renamed. This is not possible. And this checks might get complicated (grass had 3 renames). It's much easier to a single method for getting correct data in this case. Also that pr doesn't use features.

zardoy avatar Jan 21 '24 03:01 zardoy

So I don't see any benefits of not using it.

Also we can make things even simpler with something like this:

const mcData = require('minecraft-data')

const targetVersion = '1.13.1'

const sourceVersion = '1.20.1'
const blocks = new Proxy({}, {
    get(t, p) {
        return mcData.getRenamedData('blocks', p, sourceVersion, targetVersion)
    }
})

// no need to worry about renames across versions
const grassBlock = blocks.grass_block

zardoy avatar Jan 21 '24 03:01 zardoy

I think that's pretty bad. If there are changes between versions they need to be correctly handled properly, not like that. The more correct way to do this would be to have unique custom names for blocks between versions. But otherwise, we'd have to do something like this on all Minecraft-data accesses that depends on the name field everywhere, and it would be both slow and hacky.

extremeheat avatar Jan 21 '24 03:01 extremeheat

If there are changes between versions they need to be correctly handled properly, not like that

I'm not saying that you shouldn't not use features at all. But in case blocks and items they only get renamed across versions and that's it (if we don't take the flattening into account). You can see in the json file in minecraft-data that there are a lot renames.

Oh, yes, agree, this example with Proxy should be improved in terms of performance (e.g. by using a map as you said). But I don't understand why you think its hacky

zardoy avatar Jan 21 '24 03:01 zardoy

I mean I don't see any problems with the function in this pr and having that json file in mc-data pr. There is no alternatives for now (features don't have info about all blocks). If you still don't like something can you provide a specific example? Let's see what we can improve here..

zardoy avatar Jan 21 '24 05:01 zardoy

If you want to have an unique names for blocks that works across version then we can consider introducing it if there is no semantic change between blocks that have different names but same unique names.

This renaming method is like a diff and we decided a long time ago not to represent the data as diff in Minecraft data because it causes a lot of complexity for little gain. Having all the data be flat and explicit is much easier.

So next steps here

  • Figure out if indeed these changes did not have any functional change. (That seems really unlikely)
  • if it's really the case then introduce unique names in blocks.json files

rom1504 avatar Jan 21 '24 07:01 rom1504

tallgrass -> grass, grass -> short_grass (from oldest to newest) or yellow_wool -> wool

You example itself is already showing that this renaming is not without semantic changes.

So really this is not a renaming. This is deleted blocks and created blocks that are related.

I'd suggest you rather figure out some "group of blocks" that have similar properties and store that information in a file.

For example we may want to handle all chests in a similar way, all grass in a similar way etc.

rom1504 avatar Jan 21 '24 07:01 rom1504

You example itself is already showing that this renaming is not without semantic changes.

I have already said above that only the flattening changes the semantic meaning of many blocks e.g. many blocks were merged. But for all other versions, it's just renaming e.g. in tallgrass -> grass, grass -> short_grass there are no semantic meaning changes. So most changes all just simple name changes, but even for 1.13, this is something that you should keep in mind when using this method. And as I understand your proposal is to remove such renames like yellow_wool -> wool? Well, in this case it would be much more complicated for me as the the idea itself of getting the same wool block would not work for some versions (eg 1.13).

if it's really the case then introduce unique names in blocks.json files

but wouldn't it be harder to maintain since we would need to additionally process all blocks somehow (and as I understand it would manual process?). Also can you provide an example for the same grass block

I'd suggest you rather figure out some "group of blocks" that have similar properties and store that information in a file.

I was thinking about it some time ago, the easiest way to group them is to use block or item class, however there is no problem in using filters like name.endsWith('_pressure_plate') or similar. Also I don't understand how would solve the issue here. I don't need to group all the chests. I just need to get the block that simply exists in any version after the specific one e.g. the same short grass block.

zardoy avatar Jan 21 '24 09:01 zardoy

I just need to get the block which block? How do you know you want this one? What are some examples of use cases?

rom1504 avatar Jan 21 '24 09:01 rom1504

tallgrass -> grass, grass -> short_grass

This is one example that

  • Has a semantic change (tall vs neutral vs short)
  • there are multiple blocks with that semantic in the latest version (short tall and other variants)

So the rename pattern is not working well for this case

rom1504 avatar Jan 21 '24 09:01 rom1504

I just need to get the block which block? How do you know you want this one? What are some examples of use cases?

Just look at diamond-square for example. It just switches between different default block states (e.g. uses different block names like grass). I also wanted to use this in the following scenarios:

  • normalize user input e.g. setblock that would translate new block names to the block names of the current version (if no such no block in the current version). However I think remaps like yellow_wool -> wool would not make sense, but we can just remove such?
  • world generators that spawn items / blocks with default state ids. these scripts might use some external data
  • I'm using it along with only the latest block collision data

Again, for 1.13 and above we can just use these renames without any additional handling. For 1.13 specifically, there is another mapping in mc data. Am I not right?

Has a semantic change (tall vs neutral vs short)

I actually thought its the same block

zardoy avatar Jan 23 '24 00:01 zardoy

diamond square should be using support feature, like this for example https://github.com/PrismarineJS/diamond-square/pull/19/files

trying to map everything to one version definitely is not the approach we are taking here; and would make the code overall much less robust

rom1504 avatar Jan 27 '24 20:01 rom1504

for pre/post flattening, mapping the metadata can be useful; in particular for viewing. What about fixing this file? we already discussed how to do it

rom1504 avatar Jan 27 '24 20:01 rom1504

trying to map everything to one version definitely is not the approach we are taking here; and would make the code overall much less robust

why less robust? I think it makes it more robust instead (eg new versions that introduce renames like with short grass block won't break anything). Can you give an example? Original client already has data fixers built-in, why don't you want to have a similar thing here? It's just super handy in some scenarios like when you just want to have a default state of some mappable block (diamond square code you linked could much more simple).

for pre/post flattening

Yes I already mentioned it a few times above, that file has nothing to do with other renames for other versions. However, maybe we can merge these data into one file so we have a complete and more structured solution? I just want to understand what you don't like here.. There is a big number of renames items / blocks. It would be tedious to support for each of them to supportFeature and would be much easier to just use one interface that would output the correct name for your version instead of having to keep in mind all possible renames for all versions.

zardoy avatar Jan 29 '24 12:01 zardoy

I think we need to properly support all blocks instead of doing renames that are actually not the same blocks

That's what I mean by not robust. You map to another version but

  1. The semantic are lost in the mapping
  2. What is the target version? Why support only that version instead of a common representation independent of the latest version supported ?

rom1504 avatar Jan 29 '24 12:01 rom1504

I think we need to properly support all blocks instead of doing renames that are actually not the same blocks

Of course we don't want to have renames maps of different blocks. Only blocks that have changed their names without semantic change. That's why I'm going to remove data for 1.13 to ensure legacy.json is also used properly. Again, for versions above 1.13 there are a couple of block/item renames and none of them have semantic changes AFAIK.

What is the target version? Why support only that version instead of a common representation independent of the latest version supported ?

Also didn't get the question. The block data is different between versions, that's why there are versionFrom and versionTo parameters. For example, a user script can be old and contain block names for 1.14 and the bot or server might be running with the version 1.20 selected

zardoy avatar Feb 04 '24 23:02 zardoy

As discussed, these renames have many anti patterns.

Could you try to instead implement and use "block group" which would be a mapping per version from block name to name of group. It would act a bit like material property and would allow you to treat different blocks with similar properties in the same way ?

rom1504 avatar Feb 05 '24 09:02 rom1504

@zardoy please send examples of usage where renames make sense and groups don't

I still can't think of any reasonable usage of renames. Especially for the fact it forces the user code to pretend to use the wrong version

rom1504 avatar Feb 12 '24 08:02 rom1504