Skip to content

Add further support for bedrock's dyed icon variant in the custom item API#12

Open
ezariago wants to merge 1 commit into
eclipseisoffline:dyeable-item-apifrom
ezariago:dyeable-item-api
Open

Add further support for bedrock's dyed icon variant in the custom item API#12
ezariago wants to merge 1 commit into
eclipseisoffline:dyeable-item-apifrom
ezariago:dyeable-item-api

Conversation

@ezariago
Copy link
Copy Markdown

This PR includes the required code changes for the dyed icon variant of bedrock's dyeable item.

The PR also changes data structure on mappings to use a component-based object (instead of simple hex integer) for bedrock_options, which now is written as follows:

"bedrock_options": {
  "dyeable": {
    "default_color": "aceace"
    "dyed_icon": "cloth:tshirt_dyed"
  }
}

This is done to ensure all dyeable-related properties are grouped under a single object for better consistency.

@eclipseisoffline
Copy link
Copy Markdown
Owner

Will try to review this some time soon™ - not sure whether I like that dyedIcon is another new property of CustomItemBedrockOptions, in my opinion it could be nicer to have icons grouped under a single interface to be honest. This would allow easier adding of the other icon types in the future and keeps them all nicely together.

Also, have you tested this actually works? I'm not sure if the dyed icon property is sent over network the same as it appears in behaviour packs.

@ezariago
Copy link
Copy Markdown
Author

Will try to review this some time soon™

Tysm! Sorry for the late reply

not sure whether I like that dyedIcon is another new property of CustomItemBedrockOptions, in my opinion it could be nicer to have icons grouped under a single interface to be honest. This would allow easier adding of the other icon types in the future and keeps them all nicely together.

Sure thing! I noticed the same thing while I was writing the code, where the dyedIcon property feels a little off the place. I'll write the revision in a few minutes

Also, have you tested this actually works? I'm not sure if the dyed icon property is sent over network the same as it appears in behaviour packs.

As for the dyed_icon, I haven't done much testing about it 🙃 because I'm still working on getting the normal icon works on my server :P The only thing fully tested was the minecraft:dyeable component from your original PR. I just started learning about bedrock inner workings a few days ago hihihi. Will also test this once I got my Items working

@eclipseisoffline
Copy link
Copy Markdown
Owner

Thanks! If you don't feel like doing it, I can also do a quick test with dyed_icon when I get to reviewing this, that shouldn't be a lot of effort for me. Just let me know!

@ezariago
Copy link
Copy Markdown
Author

If you don't mind doing a quick test about it, that would be great! :D it's already getting late for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants