Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add getRenamedData #335

Closed
wants to merge 4 commits into from
Closed

add getRenamedData #335

wants to merge 4 commits into from

Conversation

zardoy
Copy link
Contributor

@zardoy zardoy commented Jan 18, 2024

ref PrismarineJS/minecraft-data#836, really want to have it!

@@ -43,3 +43,5 @@ console.log(getMcData.supportedVersions.pc)
console.log(getMcData.schemas.blocks)

console.log(getMcData('1.12').language['options.sensitivity.max'])
console.log(getMcData.getRenamedData('blocks', 'short_grass', '1.20.3', '1.8.8') === 'tallgrass') // test string comparison
console.log(getMcData.getRenamedData('blocks', ['standing_sign', 'grass'], '1.8.8', '1.16'))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will be [ 'oak_sign', 'grass_block' ]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an example of calling the API but not an example of usage. I don't think we should ever try to do that.

Clearly tall and short are not the same thing

Oak is a specific kind of wood

Grass and grass block may be different

Having tags and groups make sense to note similar blocks that should be treated similarly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having tags and groups make sense to note similar blocks that should be treated similarly

It depends on the use case. As I mentioned earlier there are a lot of use cases when you want to track block renames across the versions (such as world upgrades and so on).

Here short_grass and tallgrass are the same thing actually (dont' let 1.8.8 naming fool you)

@zardoy
Copy link
Contributor Author

zardoy commented Jan 21, 2024

diamond-square and flying-squid will definitely benefit from it:
PrismarineJS/diamond-square#15

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

@extremeheat
Copy link
Member

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
Copy link
Contributor Author

zardoy commented Jan 21, 2024

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
Copy link
Contributor Author

zardoy commented Jan 21, 2024

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
Copy link
Member

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
Copy link
Contributor Author

zardoy commented Jan 21, 2024

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
Copy link
Contributor Author

zardoy commented Jan 21, 2024

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
Copy link
Member

rom1504 commented Jan 21, 2024

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
Copy link
Member

rom1504 commented Jan 21, 2024

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
Copy link
Contributor Author

zardoy commented Jan 21, 2024

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
Copy link
Member

rom1504 commented Jan 21, 2024

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
Copy link
Member

rom1504 commented Jan 21, 2024

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

@zardoy
Copy link
Contributor Author

zardoy commented Jan 23, 2024

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

@rom1504
Copy link
Member

rom1504 commented Jan 27, 2024

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
Copy link
Member

rom1504 commented Jan 27, 2024

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
Copy link
Contributor Author

zardoy commented Jan 29, 2024

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
Copy link
Member

rom1504 commented Jan 29, 2024

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
Copy link
Contributor Author

zardoy commented Feb 4, 2024

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
Copy link
Member

rom1504 commented Feb 5, 2024

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
Copy link
Member

rom1504 commented Feb 12, 2024

@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 rom1504 closed this Oct 12, 2024
@zardoy zardoy deleted the renamed-data branch October 12, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants