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

Make hair styles conform to using variants for colors #70017

Merged
merged 14 commits into from
Dec 7, 2023

Conversation

furran
Copy link
Contributor

@furran furran commented Dec 5, 2023

Summary

Bugfixes "Make hair styles conform to using variants for colors"

Purpose of change

The purpose of this change is to make all hairstyles conform to using variants for color

Describe the solution

  • Made all hairstyles use variants for coloring
  • Made the necessary migration as to avoid throwing errors at the player and making them bald in the process
  • Edited the jsons that used the old hairstyles to use the new hairstyle ids and variants
  • Edited the tile_config files for every tileset to use the new hairstyle ids and variants

Describe alternatives you've considered

Considered doing the opposite and undoing variant for hairstyles and eye_color, but that felt like going backwards.

Testing

Save file from 0.G: Debug-trimmed.tar.gz

Made a few tests:

  1. Created a new character and selected one of the hairstyles that now conform to the variants color style ('fro black) and checked in game through @ and the sprite (tileset used: GiantDays).

  2. Created a character in the stable 0.G version with the black 'fro hairstyle and exported the save to this build. Loaded the save, observed if there were any error messages related to the changes ( there were some unrelated) and checked if my character still had the same hairstyle. Tested the same thing with the hairstyle short_no_fringe black variant.

Additional context

Some observations:

  • Some tilesets have sprites for some hairstyles while others don't. Keep that in mind if testing that.
  • mutation migrations don't seem to support "from_variant", so if the player character is using a re-identified hairstyle that had variants the migration will default the player hair color to black.

 - Check if selected trait has variations when in a customization menu
 and if it does, show the variation menu to select a variation.
@github-actions github-actions bot added [JSON] Changes (can be) made in JSON SDL: Tiles / Sound Tiles visual interface and sounds. Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies [C++] Changes (can be) made in C++. Previously named `Code` <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Dec 6, 2023
@furran furran changed the title Customize appearance Fix in-game customization and make hair styles use variants for colors Dec 6, 2023
@github-actions github-actions bot added NPC / Factions NPCs, AI, Speech, Factions, Ownership Mods Issues related to mods or modding Mods: Aftershock Anything to do with the Aftershock mod Mods: Xedra Evolved Anything to do with Xedra Evolved and removed json-styled JSON lint passed, label assigned by github actions labels Dec 6, 2023
@github-actions github-actions bot added the json-styled JSON lint passed, label assigned by github actions label Dec 6, 2023
@furran furran marked this pull request as ready for review December 6, 2023 02:44
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Auto-requesting reviews from non-collaborators: @GuardianDll

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 6, 2023
@AMurkin
Copy link
Contributor

AMurkin commented Dec 6, 2023

I suggest some way of indication of menu items which will prompt for a variant.
Something like this:

diff --git a/src/mutation.cpp b/src/mutation.cpp
index 19451fc063..cafb3fb023 100644
--- a/src/mutation.cpp
+++ b/src/mutation.cpp
@@ -2363,13 +2363,9 @@ void Character::customize_appearance( customize_appearance_choice choice )
                 current_trait = trait;
                 char_has_trait = true;
             }
-
-            const std::string &entry_name = mutation_name( trait );
-
-            amenu.addentry(
-                i, true, MENU_AUTOASSIGN,
-                char_has_trait ? entry_name + " *" : entry_name
-            );
+            amenu.addentry( i, true, MENU_AUTOASSIGN,
+                            ( trait->variants.empty() ? mutation_name( trait ) : string_format( _( "Variant of %s… " ),
+                                    trait->name() ) ) + ( char_has_trait ? " *" : "" ) );
         }
     };

@furran
Copy link
Contributor Author

furran commented Dec 6, 2023

I suggest some way of indication of menu items which will prompt for a variant. Something like this:

diff --git a/src/mutation.cpp b/src/mutation.cpp
index 19451fc063..cafb3fb023 100644
--- a/src/mutation.cpp
+++ b/src/mutation.cpp
@@ -2363,13 +2363,9 @@ void Character::customize_appearance( customize_appearance_choice choice )
                 current_trait = trait;
                 char_has_trait = true;
             }
-
-            const std::string &entry_name = mutation_name( trait );
-
-            amenu.addentry(
-                i, true, MENU_AUTOASSIGN,
-                char_has_trait ? entry_name + " *" : entry_name
-            );
+            amenu.addentry( i, true, MENU_AUTOASSIGN,
+                            ( trait->variants.empty() ? mutation_name( trait ) : string_format( _( "Variant of %s… " ),
+                                    trait->name() ) ) + ( char_has_trait ? " *" : "" ) );
         }
     };

I don't know, I feel like it feels cleaner and clearer without it. But I probably should follow your example and use trait->name if it has variants.

A B
image image

Copy link
Contributor

@ehughsbaird ehughsbaird left a comment

Choose a reason for hiding this comment

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

I don't think tilesets are updated here, they're updated at https://github.com/I-am-Erk/CDDA-Tilesets

It would probably better if changing the hair to use variants and the fix to the customization were in separate PRs - they're not really related, and one of them is really big.

@furran
Copy link
Contributor Author

furran commented Dec 6, 2023

I don't think tilesets are updated here, they're updated at https://github.com/I-am-Erk/CDDA-Tilesets

I thought changes to the tileset sprites/images are made at https://github.com/I-am-Erk/CDDA-Tilesets but changes to things like tile_config.json are made here? I might be wrong though

It would probably better if changing the hair to use variants and the fix to the customization were in separate PRs - they're not really related, and one of them is really big.

You are right. I'm moving the customization fix to it's own PR

@furran furran changed the title Fix in-game customization and make hair styles use variants for colors Make hair styles conform to using variants for colors Dec 6, 2023
@github-actions github-actions bot added BasicBuildPassed This PR builds correctly, label assigned by github actions and removed BasicBuildPassed This PR builds correctly, label assigned by github actions labels Dec 6, 2023
@Maleclypse Maleclypse merged commit aa3f81b into CleverRaven:master Dec 7, 2023
25 checks passed
@ehughsbaird
Copy link
Contributor

I don't think tilesets are updated here, they're updated at https://github.com/I-am-Erk/CDDA-Tilesets

I thought changes to the tileset sprites/images are made at https://github.com/I-am-Erk/CDDA-Tilesets but changes to things like tile_config.json are made here? I might be wrong though

The tile configs seem to be completely overwritten in #69886

@furran
Copy link
Contributor Author

furran commented Dec 7, 2023

@ehughsbaird ouch. You are right. Gonna open an issue in that repo informing them of these changes to hair id.

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) [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Mods: Aftershock Anything to do with the Aftershock mod Mods: Xedra Evolved Anything to do with Xedra Evolved Mods Issues related to mods or modding Mutations / Traits / Professions/ Hobbies Mutations / Traits / Professions/ Hobbies NPC / Factions NPCs, AI, Speech, Factions, Ownership SDL: Tiles / Sound Tiles visual interface and sounds.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants