PrismarineJS / node-minecraft-data

Provide easy access to minecraft-data in node.js
https://prismarinejs.github.io/minecraft-data/
100 stars 55 forks source link

add getRenamedData #335

Closed zardoy closed 2 weeks ago

zardoy commented 9 months ago

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

zardoy commented 9 months ago

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)

extremeheat commented 9 months ago

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

zardoy commented 9 months ago

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 commented 9 months ago

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
extremeheat commented 9 months ago

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.

zardoy commented 9 months ago

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 commented 9 months ago

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..

rom1504 commented 9 months ago

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

rom1504 commented 9 months ago

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.

zardoy commented 9 months ago

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.

rom1504 commented 9 months ago

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 commented 9 months ago

tallgrass -> grass, grass -> short_grass

This is one example that

So the rename pattern is not working well for this case

zardoy commented 9 months ago

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:

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

rom1504 commented 9 months ago

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 commented 9 months ago

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

zardoy commented 9 months ago

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.

rom1504 commented 9 months ago

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 ?
zardoy commented 8 months ago

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

rom1504 commented 8 months ago

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 commented 8 months ago

@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