VoxeLibre/VoxeLibre
VoxeLibre
/
VoxeLibre
57
134
Fork 173
Code Issues 935 Pull Requests 78 Projects 4 Releases Wiki Activity

add 1.20 armor trims #3784

Merged
chmodsayshello merged 39 commits from armor_trims into master 2023-10-03 23:46:06 +02:00
Conversation 26 Commits 39 Files Changed 78 +236 -29
chmodsayshello commented 2023-06-07 16:49:02 +02:00
Member
Copy Link

This pull request adds the Minecraft 1.20 armor trims to mineclone

Note

This pull request changes A LOT of files, however, most of them are textures (59), as well as one line changes in loot tables etc

To be done

  • Only apply to one piece of armor

  • add templates

  • add template "duplication" recipie

  • alter smithing table

  • add color support

  • add inventory image

  • remove from creative inventory

  • fix itemstring of enchanted items set enchanted variant to trimmed

  • move mcl_armor_trims into mcl_armor

  • resolve merge conflicts

Testing

  • get various minerals, armor templates, armor pieces and a smithing table
  • apply the trims using the smithing table
  • verify the armor now renders differently (don't forget armor stands!)
  • verify you can still upgrade diamond tools to netherite like before
  • check that you can still see the armor trims on players in minetest below 5.7
  • check better inventory image rendering in 5.8+

Thanks to @cora for providing the texture of the "trim shadow" used in the smithing table

This pull request adds the Minecraft 1.20 armor trims to mineclone ### Note This pull request changes A LOT of files, however, most of them are textures (59), as well as one line changes in loot tables etc ### To be done - [x] Only apply to one piece of armor - [x] add templates - [x] add template "duplication" recipie - [x] alter smithing table - [x] add color support - [x] add inventory image - [x] remove from creative inventory - [x] ~~fix itemstring of enchanted items~~ set enchanted variant to trimmed - [x] move mcl_armor_trims into mcl_armor - [x] resolve merge conflicts ### Testing * get various minerals, armor templates, armor pieces and a smithing table * apply the trims using the smithing table * verify the armor now renders differently (don't forget armor stands!) * verify you can still upgrade diamond tools to netherite like before * check that you can still see the armor trims on players in minetest below 5.7 * check better inventory image rendering in 5.8+ Thanks to @cora for providing the texture of the "trim shadow" used in the smithing table
❤️ 1 🎉 1 🚀 2
chmodsayshello self-assigned this 2023-06-07 16:49:03 +02:00
chmodsayshello added 4 commits 2023-06-07 16:49:09 +02:00
chmodsayshello added 1 commit 2023-06-07 17:07:19 +02:00
chmodsayshello added 1 commit 2023-06-07 17:12:26 +02:00
chmodsayshello added 1 commit 2023-06-07 17:23:16 +02:00
chmodsayshello added 1 commit 2023-06-07 19:48:27 +02:00
chmodsayshello added 3 commits 2023-06-07 20:32:17 +02:00
chmodsayshello added 1 commit 2023-06-07 21:02:31 +02:00
chmodsayshello added 1 commit 2023-06-07 22:28:24 +02:00
chmodsayshello added 2 commits 2023-06-07 22:49:23 +02:00
chmodsayshello added 1 commit 2023-06-08 00:17:37 +02:00
chmodsayshello added 1 commit 2023-06-08 10:02:57 +02:00
chmodsayshello added 1 commit 2023-06-08 10:06:45 +02:00
chmodsayshello changed title from WIP: add 1.20 armor trims to add 1.20 armor trims 2023-06-08 10:09:03 +02:00
chmodsayshello added 1 commit 2023-06-08 10:39:07 +02:00
chmodsayshello added 1 commit 2023-06-08 10:42:05 +02:00
kneekoo commented 2023-06-09 18:20:15 +02:00
Contributor
Copy Link

I just started testing this, and I noticed the following images can be optimized for a lower file size.

5.3 KiB textures/boots_trim.png
5.4 KiB textures/chestplate_trim.png
6.3 KiB textures/coast_armor_trim_smithing_template.png
6.4 KiB textures/dune_armor_trim_smithing_template.png
6.2 KiB textures/eye_armor_trim_smithing_template.png
6.0 KiB textures/eye_leggings.png
5.3 KiB textures/helmet_trim.png
5.4 KiB textures/leggings_trim.png
6.1 KiB textures/rib_armor_trim_smithing_template.png
6.1 KiB textures/sentry_armor_trim_smithing_template.png
6.0 KiB textures/snout_armor_trim_smithing_template.png
6.1 KiB textures/spire_armor_trim_smithing_template.png
6.1 KiB textures/tide_armor_trim_smithing_template.png
6.1 KiB textures/ward_armor_trim_smithing_template.png
6.2 KiB textures/wild_armor_trim_smithing_template.png

For everyone else testing, some extra info:

Armor trim Where to find it Comment
coast shipwrecks -
dune desert temple -
eye - should be in strongholds (I never went to one)
rib nether fortress -
sentry pillager outpost -
snout - should be in bastion remnants, which we don't have yet
spire end city -
tide elder guardian drop can be very hard to find
vex woodland mansion -
ward - should be in ancient cities, which we don't have yet
wild jungle temple -
I just started testing this, and I noticed the following images [can be optimized](https://git.minetest.land/MineClone2/MineClone2/src/branch/master/TEXTURES.md#further-optimization-with-optipng) for a lower file size. ``` 5.3 KiB textures/boots_trim.png 5.4 KiB textures/chestplate_trim.png 6.3 KiB textures/coast_armor_trim_smithing_template.png 6.4 KiB textures/dune_armor_trim_smithing_template.png 6.2 KiB textures/eye_armor_trim_smithing_template.png 6.0 KiB textures/eye_leggings.png 5.3 KiB textures/helmet_trim.png 5.4 KiB textures/leggings_trim.png 6.1 KiB textures/rib_armor_trim_smithing_template.png 6.1 KiB textures/sentry_armor_trim_smithing_template.png 6.0 KiB textures/snout_armor_trim_smithing_template.png 6.1 KiB textures/spire_armor_trim_smithing_template.png 6.1 KiB textures/tide_armor_trim_smithing_template.png 6.1 KiB textures/ward_armor_trim_smithing_template.png 6.2 KiB textures/wild_armor_trim_smithing_template.png ``` For everyone else testing, some extra info: | Armor trim | Where to find it | Comment | | - | - | - | | coast | shipwrecks | - | | dune | desert temple | - | | eye | - | should be in strongholds *(I never went to one)* | | rib | nether fortress | - | | sentry | pillager outpost | - | | snout | - | should be in bastion remnants, which we don't have yet | | spire | end city | - | | tide | elder guardian drop | can be very hard to find | | vex | woodland mansion | - | | ward | - | should be in ancient cities, which we don't have yet | | wild | jungle temple | - |
chmodsayshello added 1 commit 2023-06-17 20:17:03 +02:00
chmodsayshello commented 2023-06-17 20:17:59 +02:00
Author
Member
Copy Link

I just started testing this, and I noticed the following images can be optimized for a lower file size.

5.3 KiB textures/boots_trim.png
5.4 KiB textures/chestplate_trim.png
6.3 KiB textures/coast_armor_trim_smithing_template.png
6.4 KiB textures/dune_armor_trim_smithing_template.png
6.2 KiB textures/eye_armor_trim_smithing_template.png
6.0 KiB textures/eye_leggings.png
5.3 KiB textures/helmet_trim.png
5.4 KiB textures/leggings_trim.png
6.1 KiB textures/rib_armor_trim_smithing_template.png
6.1 KiB textures/sentry_armor_trim_smithing_template.png
6.0 KiB textures/snout_armor_trim_smithing_template.png
6.1 KiB textures/spire_armor_trim_smithing_template.png
6.1 KiB textures/tide_armor_trim_smithing_template.png
6.1 KiB textures/ward_armor_trim_smithing_template.png
6.2 KiB textures/wild_armor_trim_smithing_template.png

done :)

> I just started testing this, and I noticed the following images [can be optimized](https://git.minetest.land/MineClone2/MineClone2/src/branch/master/TEXTURES.md#further-optimization-with-optipng) for a lower file size. > > ``` > 5.3 KiB textures/boots_trim.png > 5.4 KiB textures/chestplate_trim.png > 6.3 KiB textures/coast_armor_trim_smithing_template.png > 6.4 KiB textures/dune_armor_trim_smithing_template.png > 6.2 KiB textures/eye_armor_trim_smithing_template.png > 6.0 KiB textures/eye_leggings.png > 5.3 KiB textures/helmet_trim.png > 5.4 KiB textures/leggings_trim.png > 6.1 KiB textures/rib_armor_trim_smithing_template.png > 6.1 KiB textures/sentry_armor_trim_smithing_template.png > 6.0 KiB textures/snout_armor_trim_smithing_template.png > 6.1 KiB textures/spire_armor_trim_smithing_template.png > 6.1 KiB textures/tide_armor_trim_smithing_template.png > 6.1 KiB textures/ward_armor_trim_smithing_template.png > 6.2 KiB textures/wild_armor_trim_smithing_template.png > ``` > done :)
ancientmarinerdev requested changes 2023-06-18 18:47:10 +02:00
ancientmarinerdev left a comment
Owner
Copy Link

Thanks for raising the PR. Added a few items of feedback.

Thanks for raising the PR. Added a few items of feedback.
mods/ITEMS/mcl_armor_trims/templates.lua Outdated
@ -0,0 +2,4 @@
local S = minetest.get_translator(modname)
for _, template_name in pairs(mcl_armor_trims.overlays) do
minetest.register_craftitem(modname .. ":" .. template_name, {
ancientmarinerdev commented 2023-06-18 18:22:54 +02:00
Owner
Copy Link

This string append is done 3 times: modname .. ":" .. template_name

It might be better to append in a local variable, and use that. Makes it easier to debug too.

This string append is done 3 times: modname .. ":" .. template_name It might be better to append in a local variable, and use that. Makes it easier to debug too.
ancientmarinerdev commented 2023-06-28 01:45:14 +02:00
Owner
Copy Link

I think you've done this in other places, but not here in this for loop. In the for loop there are 3 references, so that is logically where a local variable could be used for that.

I think you've done this in other places, but not here in this for loop. In the for loop there are 3 references, so that is logically where a local variable could be used for that.
chmodsayshello marked this conversation as resolved
mods/ITEMS/mcl_smithing_table/init.lua Outdated
@ -7,3 +7,3 @@
-- Function to upgrade diamond tool/armor to netherite tool/armor
function mcl_smithing_table.upgrade_item(itemstack)
function mcl_smithing_table.upgrade_item_netherite(itemstack)
ancientmarinerdev commented 2023-06-18 18:28:32 +02:00
Owner
Copy Link

Why are you renaming a public function? If a mod is using upgrades, it could break it, and I think there is one that does upgrade items.

Why are you renaming a public function? If a mod is using upgrades, it could break it, and I think there is one that does upgrade items.
chmodsayshello commented 2023-06-21 11:59:52 +02:00
Author
Member
Copy Link

While I didnt change the name back (due to it being misleading now), I've added a function with the old name for legacy mods... It just calls the new one

While I didnt change the name back (due to it being misleading now), I've added a function with the old name for legacy mods... It just calls the new one
👍 1
ancientmarinerdev commented 2023-06-28 01:40:08 +02:00
Owner
Copy Link

That works, and is clear. Good call.

That works, and is clear. Good call.
chmodsayshello marked this conversation as resolved
mods/ITEMS/mcl_smithing_table/init.lua Outdated
@ -54,0 +59,4 @@
if(string.find(material_name,"mesecons:")) then
material_name = "redstone"
else
material_name = material_name:gsub("_ingot","")
ancientmarinerdev commented 2023-06-18 18:37:10 +02:00
Owner
Copy Link

I see what you're doing here, and it may work now, but if for example, an item moves from it's current mod to a new mod, it would break this in a way that may be quite tricky to diagnose. It may be better to use a map to map the name to the to expected string and that way, when something gets renamed (like mesecons could to a mcl specific mod), it will be easier to find in search.

I see what you're doing here, and it may work now, but if for example, an item moves from it's current mod to a new mod, it would break this in a way that may be quite tricky to diagnose. It may be better to use a map to map the name to the to expected string and that way, when something gets renamed (like mesecons could to a mcl specific mod), it will be easier to find in search.
Ghost commented 2023-06-18 21:51:21 +02:00
First-time contributor
Copy Link

Maybe look at how I redid the smithing table upgrading here https://codeberg.org/PrairieWind/mcl_upgraded_smithing_table/src/branch/master/smithing_table.lua and here https://codeberg.org/PrairieWind/mcl_upgraded_smithing_table/src/branch/master/items.lua.

Maybe look at how I redid the smithing table upgrading here https://codeberg.org/PrairieWind/mcl_upgraded_smithing_table/src/branch/master/smithing_table.lua and here https://codeberg.org/PrairieWind/mcl_upgraded_smithing_table/src/branch/master/items.lua.
chmodsayshello commented 2023-06-21 11:58:42 +02:00
Author
Member
Copy Link

At first I wanted to do it similar to string.split...

When I read your comment, I was thinking about using string.gmatch to get a substring after the first ':' character, but now, I've just used the array

At first I wanted to do it similar to string.split... When I read your comment, I was thinking about using string.gmatch to get a substring after the first ':' character, but now, I've just used the array
chmodsayshello marked this conversation as resolved
mods/ITEMS/mcl_smithing_table/init.lua Outdated
@ -54,0 +77,4 @@
end
local function is_smithing_mineral(itemname)
if itemname == "mcl_nether:netherite_ingot"
ancientmarinerdev commented 2023-06-18 18:38:32 +02:00
Owner
Copy Link

Based on the above comment, if this is a table, you can simply check this is a value in there, and return true if it is.

Based on the above comment, if this is a table, you can simply check this is a value in there, and return true if it is.
chmodsayshello marked this conversation as resolved
ancientmarinerdev commented 2023-06-18 18:48:57 +02:00
Owner
Copy Link

Where did the image assets come from? What's the license on them?

Edit: Answer via Discord. PP and altered to match our models

Where did the image assets come from? What's the license on them? Edit: Answer via Discord. PP and altered to match our models
kneekoo commented 2023-06-18 19:21:40 +02:00
Contributor
Copy Link

I just noticed that the optipng command we recommend preserves the metadata, which we actually want out of our textures. :P So I added "-strip all" for a better optimization, and updated TEXTURES.md in PR #3806.

Example:

** Processing: wild_armor_trim_smithing_template.png
...
Output IDAT size = 226 bytes (no change)
Output file size = 283 bytes (5858 bytes = 95.39% decrease)

That's quite a significant change - I should've tested this earlier. :)

P.S. I attached the optimized textures.

I just noticed that the `optipng` command we recommend preserves the metadata, which we actually want out of our textures. :P So I added "-strip all" for a better optimization, and updated `TEXTURES.md` in PR #3806. #### Example: ``` ** Processing: wild_armor_trim_smithing_template.png ... Output IDAT size = 226 bytes (no change) Output file size = 283 bytes (5858 bytes = 95.39% decrease) ``` That's quite a significant change - I should've tested this earlier. :) P.S. I attached the optimized textures.
optipng_armor_trims.zip
6.5 KiB
chmodsayshello added 3 commits 2023-06-21 11:22:08 +02:00
chmodsayshello added 1 commit 2023-06-21 11:49:50 +02:00
chmodsayshello added 1 commit 2023-06-21 11:51:23 +02:00
chmodsayshello added 1 commit 2023-06-21 12:16:14 +02:00
chmodsayshello requested review from ancientmarinerdev 2023-06-21 12:17:59 +02:00
chmodsayshello force-pushed armor_trims from 0bf9b691bb to a159717f18 2023-06-25 11:39:32 +02:00 Compare
chmodsayshello force-pushed armor_trims from a159717f18 to 9d1840f4ca 2023-06-25 11:43:57 +02:00 Compare
chmodsayshello commented 2023-06-25 11:45:43 +02:00
Author
Member
Copy Link

Sorry for force-pushing 2 times in a row, what I just did altered some files that it wasn't supposed to, others were reduces in filesize (like intended), and some even increased, thats why I went back to that commit

Sorry for force-pushing 2 times in a row, what I just did altered some files that it wasn't supposed to, others were reduces in filesize (like intended), and some even increased, thats why I went back to that commit
chmodsayshello force-pushed armor_trims from ae0a14b102 to d346aa07ee 2023-06-29 20:47:21 +02:00 Compare
AFCMS requested review from AFCMS 2023-07-01 21:54:04 +02:00
AFCMS commented 2023-07-01 22:01:39 +02:00
Member
Copy Link

I think it would be better to implement the armor trims without registering new items. We can already use functions in item defs to dynamically determine the player armor textures (I asked @LizzyFleckenstein03 for it with this kind of stuff in mind).

We will be able to update the item image with its metadata in 5.8, so I think we should build a tt like way of updating item images using callbacks like we do for complex tooltips already.

IMO even the *_enchanted items should go away in favor of a similar approach (allowing to enchant any item like mc with commands)
if we break compatibility with old worlds completely, maybe for Minetest next breaking release?
https://github.com/minetest/minetest/pull/12614

Armor trims are a feature I like and I hope to see them soon :)

Exemple of using mcl2 functions to update player armor with functions: https://github.com/minetest-palamod/palamod/blob/master/pala_grade/paladium_skin.lua

I think it would be better to implement the armor trims without registering new items. We can already use functions in item defs to dynamically determine the player armor textures (I asked @LizzyFleckenstein03 for it with this kind of stuff in mind). We will be able to update the item image with its metadata in 5.8, so I think we should build a `tt` like way of updating item images using callbacks like we do for complex tooltips already. IMO even the `*_enchanted` items should go away in favor of a similar approach (allowing to enchant _any_ item like mc with commands) if we break compatibility with old worlds completely, maybe for Minetest next breaking release? https://github.com/minetest/minetest/pull/12614 Armor trims are a feature I like and I hope to see them soon :) Exemple of using mcl2 functions to update player armor with functions: https://github.com/minetest-palamod/palamod/blob/master/pala_grade/paladium_skin.lua
👍 1 ❤️ 1
chmodsayshello commented 2023-07-01 23:16:15 +02:00
Author
Member
Copy Link

I think it would be better to implement the armor trims without registering new items. We can already use functions in item defs to dynamically determine the player armor textures (I asked @LizzyFleckenstein03 for it with this kind of stuff in mind).

We will be able to update the item image with its metadata in 5.8, so I think we should build a tt like way of updating item images using callbacks like we do for complex tooltips already.

IMO even the *_enchanted items should go away in favor of a similar approach (allowing to enchant any item like mc with commands)
if we break compatibility with old worlds completely, maybe for Minetest next breaking release?
https://github.com/minetest/minetest/pull/12614

Armor trims are a feature I like and I hope to see them soon :)

Exemple of using mcl2 functions to update player armor with functions: https://github.com/minetest-palamod/palamod/blob/master/pala_grade/paladium_skin.lua

I personally disagree with breaking world compatibility! There are tons of worlds and servers that are actively played since like the project's inception, and there just HAS to be a way for those to go on...

Regarding the itemmeta thing, yes, i know thats possible in minetest 5.8, but I don't think it's time to drop support for 5.7 yet either! Also, again if you want that sort of style everywhere, have fun with all the alliases, which also need to set metadata properly in order for the item to work

> I think it would be better to implement the armor trims without registering new items. We can already use functions in item defs to dynamically determine the player armor textures (I asked @LizzyFleckenstein03 for it with this kind of stuff in mind). > > We will be able to update the item image with its metadata in 5.8, so I think we should build a `tt` like way of updating item images using callbacks like we do for complex tooltips already. > > IMO even the `*_enchanted` items should go away in favor of a similar approach (allowing to enchant _any_ item like mc with commands) > if we break compatibility with old worlds completely, maybe for Minetest next breaking release? > https://github.com/minetest/minetest/pull/12614 > > Armor trims are a feature I like and I hope to see them soon :) > > Exemple of using mcl2 functions to update player armor with functions: https://github.com/minetest-palamod/palamod/blob/master/pala_grade/paladium_skin.lua I personally disagree with breaking world compatibility! There are tons of worlds and servers that are actively played since like the project's inception, and there just HAS to be a way for those to go on... Regarding the itemmeta thing, yes, i know thats possible in minetest 5.8, but I don't think it's time to drop support for 5.7 yet either! Also, again if you want that sort of style everywhere, have fun with all the alliases, which also need to set metadata properly in order for the item to work
AFCMS commented 2023-07-02 20:04:18 +02:00
Member
Copy Link

I personally disagree with breaking world compatibility! There are tons of worlds and servers that are actively played since like the project's inception, and there just HAS to be a way for those to go on...

Regarding the itemmeta thing, yes, i know thats possible in minetest 5.8, but I don't think it's time to drop support for 5.7 yet either! Also, again if you want that sort of style everywhere, have fun with all the alliases, which also need to set metadata properly in order for the item to work

IMO what should be done is implement armor trims the same way as banners (without really matching inventory icons) and then implement these icons using Minetest 5.8 API later.

What I meant by "breaking world compatibility" is Minetest making a breaking release making old worlds no longer load at all without using a converting script (no realistic possible in-game conversion code).

> I personally disagree with breaking world compatibility! There are tons of worlds and servers that are actively played since like the project's inception, and there just HAS to be a way for those to go on... > > Regarding the itemmeta thing, yes, i know thats possible in minetest 5.8, but I don't think it's time to drop support for 5.7 yet either! Also, again if you want that sort of style everywhere, have fun with all the alliases, which also need to set metadata properly in order for the item to work IMO what should be done is implement armor trims the same way as banners (without really matching inventory icons) and then implement these icons using Minetest 5.8 API later. What I meant by "breaking world compatibility" is Minetest making a breaking release making old worlds no longer load at all without using a converting script (no realistic possible in-game conversion code).
ancientmarinerdev commented 2023-07-02 23:35:56 +02:00
Owner
Copy Link

I personally disagree with breaking world compatibility! There are tons of worlds and servers that are actively played since like the project's inception, and there just HAS to be a way for those to go on...

Regarding the itemmeta thing, yes, i know thats possible in minetest 5.8, but I don't think it's time to drop support for 5.7 yet either! Also, again if you want that sort of style everywhere, have fun with all the alliases, which also need to set metadata properly in order for the item to work

IMO what should be done is implement armor trims the same way as banners (without really matching inventory icons) and then implement these icons using Minetest 5.8 API later.

What I meant by "breaking world compatibility" is Minetest making a breaking release making old worlds no longer load at all without using a converting script (no realistic possible in-game conversion code).

I don't think is on Minetest, it is on us, and we try to support old worlds as much as possible. We support the last 2 releases, so wouldn't use 5.8 code until 5.9 is out, so this is far in the future.

I don't know the inner workings of banners, but we have an existing PR, and there was a few issues flagging with either the item string or description that make me somewhat hesitant to be confident in this design.

I think chmodsayshello's solution is the best solution we have for now, and from a code perspective, I will approve it once we've validated assets. If this gets reworked next year after 5.9, I'm happy for us to discuss it then.

> > I personally disagree with breaking world compatibility! There are tons of worlds and servers that are actively played since like the project's inception, and there just HAS to be a way for those to go on... > > > > Regarding the itemmeta thing, yes, i know thats possible in minetest 5.8, but I don't think it's time to drop support for 5.7 yet either! Also, again if you want that sort of style everywhere, have fun with all the alliases, which also need to set metadata properly in order for the item to work > > IMO what should be done is implement armor trims the same way as banners (without really matching inventory icons) and then implement these icons using Minetest 5.8 API later. > > What I meant by "breaking world compatibility" is Minetest making a breaking release making old worlds no longer load at all without using a converting script (no realistic possible in-game conversion code). I don't think is on Minetest, it is on us, and we try to support old worlds as much as possible. We support the last 2 releases, so wouldn't use 5.8 code until 5.9 is out, so this is far in the future. I don't know the inner workings of banners, but we have an existing PR, and there was a few issues flagging with either the item string or description that make me somewhat hesitant to be confident in this design. I think chmodsayshello's solution is the best solution we have for now, and from a code perspective, I will approve it once we've validated assets. If this gets reworked next year after 5.9, I'm happy for us to discuss it then.
😕 1
chmodsayshello added the
Media missing
 label 2023-07-14 20:14:16 +02:00
chmodsayshello commented 2023-07-14 20:16:32 +02:00
Author
Member
Copy Link

Added media missing beacuse ALL the textures (except the ones ending with smithing_template.png) need a replacement under a compatible license

Added media missing beacuse ALL the textures (except the ones ending with `smithing_template.png`) need a replacement under a compatible license
AFCMS commented 2023-07-17 09:47:39 +02:00
Member
Copy Link

IMO what should be done is implement armor trims the same way as banners (without really matching inventory icons) and then implement these icons using Minetest 5.8 API later.

Just checked Minetest API documentation and its actually much easier than delaying the support.

Changing the inventory icon of an item just use a specific metadata field, so its completely "backward compatible" for both the server and the clients. Clients will need upcomming 5.8 to get the fancy inventory icon, clients with 5.6/5.7 will still have the tooltip description to rely on, and server can be 5.6/5.7/5.8.

5.8 release will land in less than a month so we will quickly get one of our two officially supported version able to support this feature.

https://github.com/minetest/minetest/blob/master/doc/lua_api.md#item-metadata

> IMO what should be done is implement armor trims the same way as banners (without really matching inventory icons) and then implement these icons using Minetest 5.8 API later. Just checked Minetest API documentation and its actually much easier than delaying the support. Changing the inventory icon of an item just use a specific metadata field, so its completely "backward compatible" for both the server and the clients. Clients will need upcomming 5.8 to get the fancy inventory icon, clients with 5.6/5.7 will still have the tooltip description to rely on, and server can be 5.6/5.7/5.8. 5.8 release will land in less than a month so we will quickly get one of our two officially supported version able to support this feature. https://github.com/minetest/minetest/blob/master/doc/lua_api.md#item-metadata
rudzik8 commented 2023-07-17 09:58:27 +02:00
Member
Copy Link

@AFCMS if so, does that apply to archaeology as well? I'll quote wsor here:

wsor: (https://discord.com/channels/369122544273588224/1130002139628970074)
meta aside, pretty sure your not going to be able to have all the pot variations as nodes, since minetest has a limit of 32k nodes. the most reasonable way i would think to do this is with craft items, minetest 5.8 ability to change inventory image in meta, custom on place that sets and entity instead of a node. would then use the enetities static data to keep track of its "state" and textures. instead of using regular crafting recipes, would use the on craft predict, and on craft output methods. course this is mineclone2? im assuming, so not sure if normal crafting stuff applies there

I might get it implemented and merged faster if that's true :D

https://git.minetest.land/MineClone2/MineClone2/src/branch/archaeology

@AFCMS if so, does that apply to archaeology as well? I'll quote wsor here: > **wsor:** (https://discord.com/channels/369122544273588224/1130002139628970074) > meta aside, pretty sure your not going to be able to have all the pot variations as nodes, since minetest has a limit of 32k nodes. the most reasonable way i would think to do this is with craft items, minetest 5.8 ability to change inventory image in meta, custom on place that sets and entity instead of a node. would then use the enetities static data to keep track of its "state" and textures. instead of using regular crafting recipes, would use the on craft predict, and on craft output methods. course this is mineclone2? im assuming, so not sure if normal crafting stuff applies there I might get it implemented and merged faster if that's true :D https://git.minetest.land/MineClone2/MineClone2/src/branch/archaeology
AFCMS commented 2023-07-17 20:49:20 +02:00
Member
Copy Link

@AFCMS if so, does that apply to archaeology as well? I'll quote wsor here:

wsor: (https://discord.com/channels/369122544273588224/1130002139628970074)
meta aside, pretty sure your not going to be able to have all the pot variations as nodes, since minetest has a limit of 32k nodes. the most reasonable way i would think to do this is with craft items, minetest 5.8 ability to change inventory image in meta, custom on place that sets and entity instead of a node. would then use the enetities static data to keep track of its "state" and textures. instead of using regular crafting recipes, would use the on craft predict, and on craft output methods. course this is mineclone2? im assuming, so not sure if normal crafting stuff applies there

I might get it implemented and merged faster if that's true :D

https://git.minetest.land/MineClone2/MineClone2/src/branch/archaeology

I don't really know since the inventory images for the pots are 3D, not flat file images.

> @AFCMS if so, does that apply to archaeology as well? I'll quote wsor here: > > > **wsor:** (https://discord.com/channels/369122544273588224/1130002139628970074) > > meta aside, pretty sure your not going to be able to have all the pot variations as nodes, since minetest has a limit of 32k nodes. the most reasonable way i would think to do this is with craft items, minetest 5.8 ability to change inventory image in meta, custom on place that sets and entity instead of a node. would then use the enetities static data to keep track of its "state" and textures. instead of using regular crafting recipes, would use the on craft predict, and on craft output methods. course this is mineclone2? im assuming, so not sure if normal crafting stuff applies there > > I might get it implemented and merged faster if that's true :D > > https://git.minetest.land/MineClone2/MineClone2/src/branch/archaeology I don't really know since the inventory images for the pots are 3D, not flat file images.
chmodsayshello added 1 commit 2023-08-18 16:47:51 +02:00
chmodsayshello removed the
Media missing
 label 2023-08-18 18:31:22 +02:00
ancientmarinerdev commented 2023-08-19 02:40:22 +02:00
Owner
Copy Link

Can you document the following for the assets:

  • name of texture pack if applicable, whether it's been altered
  • a link
  • license type
  • author (this one is already done, thanks)

Ideally anyone should be able to find this information in the future. I've struggled to find assets that weren't fully documented. Mod readme is probably the best place for this.

Can you document the following for the assets: - name of texture pack if applicable, whether it's been altered - a link - license type - author (this one is already done, thanks) Ideally anyone should be able to find this information in the future. I've struggled to find assets that weren't fully documented. Mod readme is probably the best place for this.
chmodsayshello commented 2023-08-19 07:48:39 +02:00
Author
Member
Copy Link

Can you document the following for the assets:

  • name of texture pack if applicable, whether it's been altered
  • a link
  • license type
  • author (this one is already done, thanks)

Ideally anyone should be able to find this information in the future. I've struggled to find assets that weren't fully documented. Mod readme is probably the best place for this.

In the past I would've agreed, however, due to textures no longer being contained inside mods themselves, I'd prefer the central credits document

> Can you document the following for the assets: > > - name of texture pack if applicable, whether it's been altered > - a link > - license type > - author (this one is already done, thanks) > > Ideally anyone should be able to find this information in the future. I've struggled to find assets that weren't fully documented. Mod readme is probably the best place for this. In the past I would've agreed, however, due to textures no longer being contained inside mods themselves, I'd prefer the central credits document
chmodsayshello added 2 commits 2023-08-27 21:45:11 +02:00
chmodsayshello changed title from add 1.20 armor trims to WIP: add 1.20 armor trims 2023-08-27 22:03:07 +02:00
chmodsayshello reviewed 2023-08-27 22:22:37 +02:00
mods/ITEMS/mcl_armor_trims/init.lua Outdated
@ -0,0 +15,4 @@
_mcl_armor_texture = function(obj, itemstack)
local overlay = itemstack:get_meta():get_string("mcl_armor_trims:trim_overlay")
local old_armor_texture = mcl_armor_trims.old_textures[itemstack:get_name()]
if type(old_armor_texture) == "function" then
chmodsayshello commented 2023-08-27 22:22:37 +02:00
Author
Member
Copy Link

i am aware that this condition is never true, I'll change that when "merging" the mod with mcl_armor

i am aware that this condition is never true, I'll change that when "merging" the mod with mcl_armor
chmodsayshello marked this conversation as resolved
chmodsayshello added 1 commit 2023-08-28 15:10:53 +02:00
chmodsayshello added 1 commit 2023-08-28 16:49:20 +02:00
chmodsayshello changed title from WIP: add 1.20 armor trims to add 1.20 armor trims 2023-08-28 16:52:26 +02:00
Wbjitscool commented 2023-08-28 16:53:48 +02:00
Member
Copy Link

Cool

Cool
chmodsayshello added 1 commit 2023-08-28 18:49:32 +02:00
cora commented 2023-08-28 19:44:09 +02:00
Contributor
Copy Link

maybe you should add a remark that an item is trimmed in the item meta description field so you can see it in inv

but otherwise looks like it works

maybe you should add a remark that an item is trimmed in the item meta description field so you can see it in inv but otherwise looks like it works
AFCMS requested changes 2023-08-31 16:31:13 +02:00
AFCMS left a comment
Member
Copy Link

I really like this feature, thanks for implementing it 👍

BLOCKERS:

Armor trims doesn't handle enchantments at this point.

I tried enchanting a trimmed armor chestplate with a book in an anvil. The item didn't have any shiny overlay, and keep crashing when you equip the armor.

More explanation in the review.

COULD HAVE:

Currently, nothing indicate which armor trim is present on an armor item in its tooltip. It can be implemented using tt.register_snippet() like enchantments.

The overlay on the inventory images is the same for all armor trims. I don't think its mandatory to have a different image for each armor trim since it represent a good amount of additional textures to do but it would be nice to have.

I really like this feature, thanks for implementing it :+1: **BLOCKERS:** Armor trims doesn't handle enchantments at this point. I tried enchanting a trimmed armor chestplate with a book in an anvil. The item didn't have any shiny overlay, and keep crashing when you equip the armor. More explanation in the review. **COULD HAVE:** Currently, nothing indicate which armor trim is present on an armor item in its tooltip. It can be implemented using [`tt.register_snippet()`](https://git.minetest.land/MineClone2/MineClone2/src/branch/master/mods/HELP/tt/API.md#tt-register_snippet-func) like enchantments. The overlay on the inventory images is the same for all armor trims. I don't think its mandatory to have a different image for each armor trim since it represent a good amount of additional textures to do but it would be nice to have.
👍 1
mods/ITEMS/mcl_armor/api.lua
@ -85,6 +85,22 @@ function mcl_armor.equip_on_use(itemstack, player, pointed_thing)
return mcl_armor.equip(itemstack, player)
end
local function get_armor_texture(textures, name, modname, itemname, itemstring)
AFCMS commented 2023-08-31 16:24:40 +02:00
Member
Copy Link

The function have to handle the enchantment overlay on the player.

Like this: fc5079258c/pala_grade/paladium_skin.lua (L37-L59)

The function have to handle the enchantment overlay on the player. Like this: https://github.com/minetest-palamod/palamod/blob/fc5079258cd3d8b76c02d200d2391b873f9f6786/pala_grade/paladium_skin.lua#L37-L59
chmodsayshello marked this conversation as resolved
mods/ITEMS/mcl_armor/api.lua Outdated
@ -88,0 +91,4 @@
mcl_armor.trims.core_textures[itemstring] = core_texture
local func = function(obj, itemstack)
local overlay = itemstack:get_meta():get_string("mcl_armor:trim_overlay")
local core_armor_texture = mcl_armor.trims.core_textures[itemstack:get_name()]
AFCMS commented 2023-08-31 16:22:28 +02:00
Member
Copy Link

core_armor_texture is undefined for enchanted items itemstrings.

Should check if the item is enchanted and take the unenchanted version of the itemstring if needed.

`core_armor_texture` is undefined for enchanted items itemstrings. Should check if the item is enchanted and take the unenchanted version of the itemstring if needed.
chmodsayshello marked this conversation as resolved
mods/ITEMS/mcl_armor/api.lua
@ -260,0 +306,4 @@
meta:set_string("inventory_image", def.inventory_image .. inv_overlay) -- dont use reload_inv_image as it's a one liner in this enviorment
end
function mcl_armor.reload_trim_inv_image(itemstack)
AFCMS commented 2023-08-31 16:29:34 +02:00
Member
Copy Link

Inventory image doesn't handle enchantment overlay.

It should check if the itemstack is enchanted and reapply it (since enchanted items have separate itemstrings, their overlays are hardcoded in the item def, so this function override them completly)

Inventory image doesn't handle enchantment overlay. It should check if the itemstack is enchanted and reapply it (since enchanted items have separate itemstrings, their overlays are hardcoded in the item def, so this function override them completly)
chmodsayshello marked this conversation as resolved
mods/ITEMS/mcl_armor/trims.lua Outdated
@ -0,0 +1,46 @@
local mod_registername = minetest.get_current_modname() .. ":"
local S = minetest.get_translator(modname)
AFCMS commented 2023-08-31 15:53:19 +02:00
Member
Copy Link

modname is undeclared

`modname` is undeclared
chmodsayshello marked this conversation as resolved
chmodsayshello added 2 commits 2023-08-31 18:03:12 +02:00
chmodsayshello commented 2023-08-31 18:05:28 +02:00
Author
Member
Copy Link

COULD HAVE:

Currently, nothing indicate which armor trim is present on an armor item in its tooltip. It can be implemented using tt.register_snippet() like enchantments.

The overlay on the inventory images is the same for all armor trims. I don't think its mandatory to have a different image for each armor trim since it represent a good amount of additional textures to do but it would be nice to have.

snippets only provide the itemstring, while this is a meta dependent story, gonna try _mcl_generate_description

> **COULD HAVE:** > > Currently, nothing indicate which armor trim is present on an armor item in its tooltip. It can be implemented using [`tt.register_snippet()`](https://git.minetest.land/MineClone2/MineClone2/src/branch/master/mods/HELP/tt/API.md#tt-register_snippet-func) like enchantments. > > The overlay on the inventory images is the same for all armor trims. I don't think its mandatory to have a different image for each armor trim since it represent a good amount of additional textures to do but it would be nice to have. snippets only provide the itemstring, while this is a meta dependent story, gonna try` _mcl_generate_description`
chmodsayshello force-pushed armor_trims from b66a16f819 to 49bd28e109 2023-08-31 18:17:01 +02:00 Compare
cora commented 2023-08-31 18:59:41 +02:00
Contributor
Copy Link

can confirm the enchanting crash is fixed and enchanting trimmed armor and trimming enchanted armor works

can confirm the enchanting crash is fixed and enchanting trimmed armor and trimming enchanted armor works
👍 1
the-real-herowl requested changes 2023-09-22 06:21:59 +02:00
the-real-herowl left a comment
Owner
Copy Link

On top of the license problems, one of the trims (sentry) resembles the MC texture too closely IMHO.

Either you contact the pack author and ask for publication of an explicit Creative Commons license, or we have to make our own textures... even I could make some stuff tbh.

Also, I'd like to see prismarine and glowstone trims, too. And golden trim should be darker.

On top of the license problems, one of the trims (sentry) resembles the MC texture too closely IMHO. Either you contact the pack author and ask for publication of an explicit Creative Commons license, or we have to make our own textures... even I could make some stuff tbh. Also, I'd like to see prismarine and glowstone trims, too. And golden trim should be darker.
LEGAL.md
@ -44,1 +44,4 @@
Armor trim models were created by Aeonix_Aeon
Source: <https://www.curseforge.com/minecraft/texture-packs/ozocraft-remix>
License: [CC BY 4.0](https://creativecommons.org/licenses/by/4.0/)
the-real-herowl commented 2023-09-22 06:19:04 +02:00
Owner
Copy Link

The license is certainly not CC-BY. It's a pinky promise license, that can at most be interpreted as CC-BY-NC, which gets flagged on CDB. However, it's not even that.

The license is certainly not CC-BY. It's a pinky promise license, that can at most be interpreted as CC-BY-NC, which gets flagged on CDB. However, it's not even that.
chmodsayshello marked this conversation as resolved
the-real-herowl commented 2023-09-22 06:23:36 +02:00
Owner
Copy Link

All the stuff from the checklist works, can confirm.

All the stuff from the checklist works, can confirm.
the-real-herowl approved these changes 2023-09-22 07:03:39 +02:00
Dismissed
the-real-herowl left a comment
Owner
Copy Link

It seems there are two licenses, the CC-BY is indeed there and is valid.

Considering other problematic textures and stuff, the sentry texture is not that big of a problem.

It seems there are two licenses, the CC-BY is indeed there and is valid. Considering other problematic textures and stuff, the sentry texture is not that big of a problem.
ancientmarinerdev commented 2023-09-22 14:48:02 +02:00
Owner
Copy Link

Comment from Discord:

"John Smith Legacy and Faithful 32 textures are used to fill in blanks"

https://www.curseforge.com/minecraft/texture-packs/faithful-32x has a custom licence

no idea what license this is: https://texture-packs.com/resourcepack/john-smith-legacy/

do we know where these textures originated?
the author won't have permission to relicense art, so the cc license will just be for their creations

Comment from Discord: "John Smith Legacy and Faithful 32 textures are used to fill in blanks" https://www.curseforge.com/minecraft/texture-packs/faithful-32x has a custom licence no idea what license this is: https://texture-packs.com/resourcepack/john-smith-legacy/ do we know where these textures originated? the author won't have permission to relicense art, so the cc license will just be for their creations
ancientmarinerdev requested changes 2023-09-22 14:48:33 +02:00
ancientmarinerdev left a comment
Owner
Copy Link

License clarification needed

License clarification needed
AFCMS commented 2023-09-22 22:09:33 +02:00
Member
Copy Link

snippets only provide the itemstring, while this is a meta dependent story, gonna try _mcl_generate_description

@chmodsayshello ItemStack is passed as the third parameter.

b4c693bb20/mods/HELP/tt/init.lua (L26)

https://github.com/minetest-palamod/palamod/blob/master/pala_legendary/endium_gauntlet.lua#L41-L61

> snippets only provide the itemstring, while this is a meta dependent story, gonna try` _mcl_generate_description` @chmodsayshello ItemStack is passed as the third parameter. https://git.minetest.land/MineClone2/MineClone2/src/commit/b4c693bb205777b39446fe631ac02823656cc26d/mods/HELP/tt/init.lua#L26 https://github.com/minetest-palamod/palamod/blob/master/pala_legendary/endium_gauntlet.lua#L41-L61
chmodsayshello added 1 commit 2023-09-26 20:25:39 +02:00
chmodsayshello dismissed the-real-herowl’s review 2023-09-26 20:25:40 +02:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

chmodsayshello requested review from AFCMS 2023-09-26 20:30:06 +02:00
chmodsayshello force-pushed armor_trims from cb196c5076 to d5522111c7 2023-09-26 22:12:00 +02:00 Compare
chmodsayshello commented 2023-09-26 22:12:51 +02:00
Author
Member
Copy Link

force pushed to remove the "b" I added by accidient while resolving the merge conflict

force pushed to remove the "b" I added by accidient while resolving the merge conflict
chmodsayshello commented 2023-09-26 22:31:43 +02:00
Author
Member
Copy Link

License clarification needed

It is CC, the textures used in mcl2 have all been made Aeonix_Aeon

> License clarification needed It is CC, the textures used in mcl2 have all been made Aeonix_Aeon
chmodsayshello force-pushed armor_trims from 3cd98a0c1e to 8936313fb3 2023-09-29 17:00:20 +02:00 Compare
chmodsayshello commented 2023-09-29 17:02:10 +02:00
Author
Member
Copy Link

Force pushed as there were still some issues related to the cherry pick, made cora's change myself at 4046a68fbf in MineClone2 history

Force pushed as there were still some issues related to the cherry pick, made cora's change myself at 4046a68fbf in **MineClone2** history
AFCMS approved these changes 2023-10-01 22:27:57 +02:00
AFCMS left a comment
Member
Copy Link

Works, good job 👍

The tooltip indications for the applied trim could be a different color from the base green one so it can be a bit easier to notice, but thats a minor problem IMO.

Works, good job :+1: The tooltip indications for the applied trim could be a different color from the base green one so it can be a bit easier to notice, but thats a minor problem IMO.
chmodsayshello merged commit 712a6d6c66 into master 2023-10-03 23:46:06 +02:00
chmodsayshello deleted branch armor_trims 2023-10-03 23:46:08 +02:00
ancientmarinerdev added this to the 0.85.0 - Fire and Stone milestone 2023-10-04 00:09:55 +02:00
ancientmarinerdev commented 2023-10-04 00:10:23 +02:00
Owner
Copy Link

Thanks for the PR, chmodsayshello. Thanks for reviewing, @AFCMS

Thanks for the PR, chmodsayshello. Thanks for reviewing, @AFCMS
❤️ 1
Sign in to join this conversation.
Reviewers
No reviewers
the-real-herowl
ancientmarinerdev
AFCMS
Labels
Clear labels   
#P1 CRITICAL

for crashes, catastrophic bugs, etc.

  
#P2: HIGH

highest priority level

  
#P3: elevated

2nd-highest priority level

  
#P4 priority: medium

for things that aren't elevated but have been prioritised

  
#P6: low

low priority level

  
#Review

Tickets that may improve or fix an issue but will need a review after a short period of time

  
annoying

non-bugs that annoys people

  
API

   
bug

   
code quality

   
combat

   
commands

   
compatibility

Label to cover migration issues from old worlds or mcl5

  
configurability

   
contribution inside

   
controls

   
core feature

   
creative mode

   
delayed for engine release

   
documentation

   
duplicate

   
enhancement

enhancement request/suggestion

  
environment

Issues that affect weather or light

  
gameplay

   
graphics

   
ground content conflict

mapgen gc-specific quirks

  
GUI/HUD

   
help wanted

   
incomplete feature

   
invalid / won't fix

Invalid, or will not fix

  
items

   
looking for contributor

   
mapgen

   
meta

Relating to VoxeLibre on Mesehub and elsewhere

  
mineclone2+

fit for MineClone2-Plus

  
Minecraft >= 1.13

   
Minecraft >= 1.17

   
missing feature

   
mobile

involving Minetest on mobile devices (android)

  
mobs

   
mod support

Issues related to mod support

  
model needed

A model is needed to be created or updated

  
multiplayer

Issues affecting multiplayer

  
Needs adoption

PR needs author action or to be adopted so changes can be implemented to get the PR closer to merge

  
needs discussion

   
needs engine change

   
needs more information

   
needs research

   
nodes

   
non-Minecraft feature

   
non-mob entities

   
performance

   
player

issues related to the player

  
possible close

   
redstone

   
release notes

This ticket will need information adding to release notes

  
schematics

   
Skyblock

   
sounds

   
Testing / Retest

This needs testing or retesting

  
tools

Related to the tools scripts, which aren't run in game

  
translation

   
unconfirmed
  
mcl5

Possibly relevant for MineClone5

  
mcla

Possibly relevant for Mineclonia

  
Media missing

media files for this feature are missing - contributions welcome

No Label
#P1 CRITICAL
#P2: HIGH
#P3: elevated
#P4 priority: medium
#P6: low
#Review
annoying
API
bug
code quality
combat
commands
compatibility
configurability
contribution inside
controls
core feature
creative mode
delayed for engine release
documentation
duplicate
enhancement
environment
gameplay
graphics
ground content conflict
GUI/HUD
help wanted
incomplete feature
invalid / won't fix
items
looking for contributor
mapgen
meta
mineclone2+
Minecraft >= 1.13
Minecraft >= 1.17
missing feature
mobile
mobs
mod support
model needed
multiplayer
Needs adoption
needs discussion
needs engine change
needs more information
needs research
nodes
non-Minecraft feature
non-mob entities
performance
player
possible close
redstone
release notes
schematics
Skyblock
sounds
Testing / Retest
tools
translation
unconfirmed
mcl5
mcla
Media missing
Milestone
Clear milestone
No items
No Milestone
0.85.0 - Fire and Stone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
chmodsayshello
9 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: VoxeLibre/VoxeLibre#3784
No description provided.
Delete Branch "armor_trims"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?