-
Notifications
You must be signed in to change notification settings - Fork 276
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
feat(mods/pride_flags): add an in-repo mod for pride flags #4362
Conversation
Wrath flags when?
Semantic PR titles continue to baffle me, hecc |
Ehh, oh well. Semantic PR is gonna get caught up over semantics |
semantic PR complains because |
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.
- transFlag
+ trans_flag
a bit of a nitpick, but generally mod id and item ids follow snake_case
convention.
it's not a rule set in stone, but being consistent is nice.
Figured, haha. I'm just used to using camel case. Will go on ahead and change it to snake_case on your suggestion |
also don't worry about semantic PR failing, it'll fail till it's merged (because the check is done using |
There, the case should now hopefully be appropriately snaked |
thanks, one more tiny nitpick: directory name isn't |
To be fair, the directory names seem to have more variance with regards to that. But I will gladly comply |
Why not I suppose
@chaosvolt i think it'll be ready to merge after loadtest, failure in semantic PR check can be ig ored (due to limitation it always fail when adding new mod) |
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.
- Load-tested with mod.
- Spawned in the last flag sprited in the mod_tileset to confirm it was the expected sprite.
Layering will as always make me screm but there's nothing one can really do to fix that, you wear your messenger bag over blankets and US flags too and the layering looks equally odd anyway:
Oops, chaos accidentally revealed that that sprite got offset on the y-axis somehow |
Wrong kind of fluid, it shouldn't be dripping of the page
Good thing Chaos happened to choose that one specific flag to test with, given that only it had its y-axis wonky xD |
@chaosvolt So sorry, could you please re-review/approve this? Your testing just happened to reveal that the genderfluid flag's sprites somehow got offset on the y-axis, so I had to fix that real quick |
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.
Purpose of change
Pride flags seemed like an appropriate thing to add in an in-repo mod and based on our community, it seemed like it would be appreciated.
Describe the solution
Describe alternatives you've considered
Making it mainline instead of a mod
Considering our lack of a variants system and how we all saw the drama that came up in the DDA PR to add pride flags to their mainline, I believe an in-repo mod is a better solution.
Adapt the DDA uncraft recipe as our crafting recipe for the pride flags
7 sheets just looked really excessive to me.
Testing
The game loads, pride flags show, the crafting is a go, and the itemgroups behave as usual.
Additional context