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

1.20 adding charms in charm slots (Trinkets "fix") #4447

Open
wants to merge 3 commits into
base: 1.20.x
Choose a base branch
from

Conversation

Shibva
Copy link

@Shibva Shibva commented Aug 28, 2023

Add hand/charm and moved divaCharm and goddessCharm to section (hopefuly I did this right)

Creates a new charm slot in the mainland section

@williewillus NOTE: needs to be run like you said; I'm still fairly new to all of these things so I'm not sure how to go about running it.

I also want to give the trinket case the needed trinket compatibility since it is missing as Curios has it but it is outside of my ability to implement such

Hopefully this will generate the other files as needed

Add hand/charm and moved 	``divaCharm`` and ``goddessCharm`` to section (hopefuly I did this right)
@Shibva
Copy link
Author

Shibva commented Aug 28, 2023

also needs backport to 1.19.2 as a reminder :D

@williewillus
Copy link
Member

You need to run ./gradlew :Fabric:runFabricDatagen (./gradlew.bat :Fabric:runFabricDatagen on windows) and commit the changes it generates.

@williewillus
Copy link
Member

williewillus commented Aug 28, 2023

Also, you're missing this change from the other PR. Is that intentional? https://github.com/VazkiiMods/Botania/pull/4443/files#diff-478bd022a79d7cb1ddd31a5f0a03f5d5ca5446fafcfe9f4abf6fc5b7e06f20f8

That part should be done manually because it's not in the generated folder

- runFabricDatagen ran wtih previous commit made above
  (NOTE:  Error message generated upon run: `com.mojang.authlib.exceptions.MinecraftClientHttpException: Status: 401`
- Added `"hand/charm"`  slot; hopefully this dosen't create a second slot with other mods that add this slot in.
- Cached Data changes from datagen run (unsure if this is needed but including just in case)

Reviews needed before approval
@Shibva
Copy link
Author

Shibva commented Aug 29, 2023

Please review this before approving it; I'll not lie when I say I only have a fundamental understanding of how the datapack aspect of this works. I will admit I do not know how to code with Java but I feel that my understanding of Minecraft's datapack system that is integrated with mods is solid enough for me to contribute in those areas. Thanks for the instructions on the datagen task.

I would also work in Trinket Case compact with trinkets but that is outside of my range of possibility

@williewillus
Copy link
Member

The code looks good to me. Did you run the game and see if it worked ingame? If so I can land this.

@Shibva
Copy link
Author

Shibva commented Aug 29, 2023

game boots but its not creatin the charm slot for some reason

@Shibva
Copy link
Author

Shibva commented Aug 29, 2023

Ok I see what needs to be done now

I'll need to add a custom slot in the same place entities are located;as in add a new file lel

I'll finish that up tomorrow when I have the chance

@Shibva
Copy link
Author

Shibva commented Aug 29, 2023

Thankfully, in the source, there's a test mod that shows me what to do

finishes the implementation

Thank You TheWinABagel for the file I copied over to finish the job
@Shibva
Copy link
Author

Shibva commented Aug 29, 2023

Ok, one look at another source later and a quick grab to make things quicker and its done.

ran it again and it now all works with the slot added and everything @williewillus

@williewillus
Copy link
Member

Hm. It doesn't seem right that we have jsons in trinkets' namespace. Let me check the docs about how this is supposed to work.

@Shibva
Copy link
Author

Shibva commented Aug 30, 2023

It should be the case; based on another mod I looked at with open source, it appears to be in the right spot

@unilock
Copy link
Contributor

unilock commented Oct 31, 2023

It's important to mention that the "charm" slot does not exist in the Trinkets API by default, although the texture for it does.

As such, the tooltip for any item that can be put in that slot will be missing a translation key; thus the following entry should be added to the lang file(s):

"trinkets.slot.hand.charm": "Charm"

For what it's worth, it seems that the most popular mod that implements a "charm" slot, Charm of Undying, defines the slot as "charm/charm", not "hand/charm". (source)

@Shibva
Copy link
Author

Shibva commented Oct 31, 2023

That is true; this is more or less a fallback in case non-such are present; the code should work, and I have tested it in the space.

@Shibva
Copy link
Author

Shibva commented Oct 31, 2023

I was told to make this to be applied for 1.20 and then retroactively for 1.19.2

@TheRealWormbo TheRealWormbo added the after-1.21-port This will not be fixed/implemented before the port to 1.21 label Jul 13, 2024
@TheRealWormbo
Copy link
Collaborator

This is worth revisiting after the 1.21 port, when hopefully the new "baubles wars" on NeoForge show hints of a winner.

@Shibva
Copy link
Author

Shibva commented Jul 13, 2024

Feel free to make the changes for it to work then as well, this was more of a nitpick fix for trinkets mod

@Shibva Shibva changed the title 1.20 adding charms in charm slots 1.20 adding charms in charm slots (Trinkets "fix") Jul 13, 2024
@Shibva
Copy link
Author

Shibva commented Jul 13, 2024

Renamed this; this is probably why it has seen no movement lel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
after-1.21-port This will not be fixed/implemented before the port to 1.21
Development

Successfully merging this pull request may close these issues.

4 participants