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

Update Magiclysm's enchanted rings armor layers and add a "SKINTIGHT" flag to vanilla rings. #60088

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

Marcos6226
Copy link
Contributor

@Marcos6226 Marcos6226 commented Aug 11, 2022

Summary

Bugfixes "Fix Magiclysm obsolete armor layers to match vanilla ones and add a ''SKINTIGHT'' flag to vanilla rings."

Purpose of change

I changed magiclysm's enchanted_rings.json armor layers to better represent the vannila ones.
I also added the "SKINTIGHT" flag to all rings in jewelry.json to partially fix conflicts with gloves.
This PR may close #60058 but I'm not entirely sure.

Describe the solution

I have added the "specifically_covers": [ "hand_fingers_l", "hand_fingers_r" ] line to make it only cover the fingers and not the entire hand. I have also added the missing "SKINTIGHT" flag to the rings so the majority of the gloves can be worn with rings.

Describe alternatives you've considered

Don't add the "SKINTIGHT" flag and find a better solution to deal with rings and gloves.

As a side note, I'm open to any suggestions and ideas.

Testing

I have tested it, and it's working as intended. The vanilla rings have the flag and it's not comflicting with normal layer gloves anymore.
Magiclysm is also working as intended. I have spawned several enchanted rings and they now only cover the fingers, and it's only possible to wear one per hand.
Below are some screenshots to prove it.

Additional context

Captura de Tela (8)

Captura de Tela (5)

@Marcos6226 Marcos6226 requested a review from KorGgenT as a code owner August 11, 2022 20:42
@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Items: Armor / Clothing Armor and clothing Mods Issues related to mods or modding Mods: Magiclysm Anything to do with the Magiclysm mod <Bugfix> This is a fix for a bug (or closes open issue) astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Aug 11, 2022
@bombasticSlacks
Copy link
Contributor

bombasticSlacks commented Aug 11, 2022

I personally think this is one of those situations where the encumbrance penalty is fine and that they shouldn't be skin tight.

It's not impossible to wear gloves and rings it's just a bit uncomfortable and I would argue that that is true to real life and accurate to what the game represents.

This isn't to dissuade you from contribution and I think other people should weigh in but that's my 2c.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Aug 11, 2022
@Marcos6226
Copy link
Contributor Author

This isn't to dissuade you from contribution and I think other people should weigh in but that's my 2c.

Thank you for sharing your opinion! I also think that the encumbrance penalty is not a huge deal; after all, vanilla rings don't give anything unless you have the stylish trait. Of course, if someone explicitly says to remove it, I'll do it with no problems whatsoever.

If you have any more suggestions, I'm all ears.

@jagoly
Copy link
Contributor

jagoly commented Aug 12, 2022

I personally think this is one of those situations where the encumbrance penalty is fine and that they shouldn't be skin tight.

Personally I think the penalty should be more like 0.5 encumbrance, but that's not possible so I think zero would be closer to accurate than the pretty significant penalty of 2 or 3. But I concede that it's probably not worth putting too much effort into making that possible with new flags or something, as the penalty isn't that strange.

I don't understand why you don't think they should be skintight. Who wears rings over gloves IRL? That just sounds strange (and impossible for all but the thinnest gloves).

@Rivet-the-Zombie Rivet-the-Zombie merged commit 07d8e66 into CleverRaven:master Aug 16, 2022
@Marcos6226 Marcos6226 deleted the BugFix-2 branch August 16, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) Items: Armor / Clothing Armor and clothing [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Mods: Magiclysm Anything to do with the Magiclysm mod Mods Issues related to mods or modding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

various issues with rings
4 participants