-
Notifications
You must be signed in to change notification settings - Fork 105
Add generateRelative
function
#58
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
Conversation
…ite-gender relative
@tomkennedy22 want to review my PR? No worries if you're too busy just lmk :) |
Would be happy to, will take a look tonight 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it works well! Nice work.
I left a few small food-for-thought comments, but all take it or leave it
|
||
// Currently, race just affects skin color and hair color. Let's ignore hair color (since you could imagine it being dyed anyway) and figure out someone's race just based on skin color. If no race is found, return undefined. | ||
const imputeRace = (face: FaceConfig) => { | ||
return races.find((race) => colors[race].skin.includes(face.body.color)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a non-issue now (and likely ever), but if multiple races share a skin color, might have conflict w/ this function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... it would also break if the default skin colors change (unless the old defaults are kept around for this function). But I think it's good enough for now, we just need to remember to deal with it if a future update introduces an issue.
src/generateRelative.ts
Outdated
}); | ||
|
||
// Always regenerate some properties | ||
face.accessories = randomFace.accessories; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder with these lines (30 ➡️ 56) if we have a master list of properties already defined, and then we just toggle some in & out of regenerate? To save from effectively defining two MECE lists back to back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to google MECE but that is a good acronym, and this is also a good point. The tricky part is getting TypeScript to enforce that the list is actually exhaustive, but I think it's possible...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty nice now! TypeScript will error if anything is ever added to FaceConfig that makes this no longer exhaustive. And it lets you choose if you want to operate on an entire object (like accessories
) or an exhaustive list of individual properties (like body
).
const regenerateProperties: RegenerateProperties = {
accessories: "always",
body: {
color: "sometimesIfRaceIsKnown",
id: "sometimes",
size: "always",
},
ear: "sometimes",
eye: "sometimes",
eyebrow: "sometimes",
eyeLine: "sometimes",
facialHair: "always",
fatness: "always",
glasses: "always",
hair: {
color: "sometimesIfRaceIsKnown",
flip: "always",
id: "always",
},
hairBg: "always",
head: {
id: "sometimes",
shave: "always",
},
jersey: "never",
miscLine: "sometimes",
mouth: "sometimes",
nose: "sometimes",
smileLine: "sometimes",
teamColors: "never",
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Clever way of doing it
@@ -53,6 +50,51 @@ const defaultTeamColors: TeamColors = ["#89bfd3", "#7a1319", "#07364f"]; | |||
|
|||
const roundTwoDecimals = (x: number) => Math.round(x * 100) / 100; | |||
|
|||
export const numberRanges = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of all of these additions :D
public/editor/TopBar.tsx
Outdated
setShuffleGenderSettingObject, | ||
setShuffleRaceSettingObject, | ||
setShuffleOtherSettingObject, | ||
} = stateStore; | ||
const genders: Gender[] = ["male", "female"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was my fault originally, but should genders & races be imported from common as well?
…in generateRelative, and have TypeScript enforce that it's exhaustive!
This PR adds a
generateRelative
function which takes an existing face object and a gender and returns a new face object that is fairly similar to the existing one, but with some differences. Basically like an immediate family member.It does this by regenerating only some properties from the input face, and others are just passed through without change. (The fraction of regenerated properties could be another parameter so you could create cousins or whatever, but I don't need that feature personally and I'm not sure if anyone else does.)
Here's an example of generating a chain of 6 relatives:
You can try it in the editor, I added a new option:
I don't love that UI because it looks kind of lonely having one option in "Other", and also because currently when you enable "Relative" it ignores the race options and the locked features. But I'm not sure what would be better.
Also I made this a separate function
generateRelative
rather than having it as another option ingenerate
because it doesn't work with the existingoverrides
andrace
options, so I think it's less confusing as a separate function.Fixes #5