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 👍 |
tomkennedy22
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
Nice! Clever way of doing it
|
|
||
| const roundTwoDecimals = (x: number) => Math.round(x * 100) / 100; | ||
|
|
||
| export const numberRanges = { |
There was a problem hiding this comment.
I'm a fan of all of these additions :D
public/editor/TopBar.tsx
Outdated
| setShuffleRaceSettingObject, | ||
| setShuffleOtherSettingObject, | ||
| } = stateStore; | ||
| const genders: Gender[] = ["male", "female"]; |
There was a problem hiding this comment.
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
generateRelativefunction 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
generateRelativerather than having it as another option ingeneratebecause it doesn't work with the existingoverridesandraceoptions, so I think it's less confusing as a separate function.Fixes #5