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

Better ter/furn copy-from support #73211

Closed
wants to merge 28 commits into from

Conversation

Procyonae
Copy link
Contributor

@Procyonae Procyonae commented Apr 23, 2024

Summary

Infrastructure "Better ter/furn copy from support, remove legacy alias code"

Purpose of change

copy-from good
Fixes #54702

Describe the solution

More copy-from support:
"symbol" supports copy-from
TRANSPARENT flag inherits correctly
"flags" support extend/delete
"connect_groups", "connects_to" and "rotates_to" support copy-from

Removes remaining redundant aliases and alias code
Removes LINE_OXOX and LINE_XOXO handling replacing it with better generic unicode character handling. Currently changes symbols to what they were already using but when I add abstracts I'll probably make most walls # instead. (The symbol only affects unconnected instances of the terrain/furniture)
Removes legacy connects_to/rotates_to flag support enabled by #73415
Merges some duplicate code in ter_t::load and furn_t::load into map_data_common_t::load

Will attempt to ensure you can overwrite "bash"'s, "deconstruct"'s and "pry"'s "ter_set"/"result" without redefining the other values for stuff like doors where that's the main difference while allowing ie "bash": { } to correctly default all the fields inherited.
May add tests to check stuff like the above works.
Will likely update inheritance docs Nvm I forgot how atrocious our existing inheritance docs were for a moment

Was originally going to add abstract terrains/furniture to cut alot of json bloat but I'll do that in future PRs bc it was getting too big to review

May remove legacy connects_to/rotates_to flag support if applying the necessary JSON changes in this PR isn't too much otherwise I'll leave it till after the follow-up PRs adding copy-from to alot of existing ter/furn
Will add abstract versions of basic terrain (probably just walls/windows/doors/floors/roofs for now)
Moves painted walls to their own file bc they were taking up half the file
Corrects several discrepancies between similar terrains

Describe alternatives you've considered

Deduplicating parts of ter_t::check and furn_t::check into map_data_common_t::check but map_data_common_t doesn't have the respective ids so the debug messages would be less useful.
I found an extern list of traps I missed while doing #72766 and #72834 which I'll remove in a separate PR

Testing

Additional context

I'd still like to make paint metadata instead of needing to have individual terrains/furniture but the save/load side of things isn't easy

NTS: Check using copy-from with an id of the wrong type (ter with furn or vice versa) throws an error. (While this could theoretically be supported I don't think it would be a good idea)

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON Map / Mapgen Overmap, Mapgen, Map extras, Map display [C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. <Bugfix> This is a fix for a bug (or closes open issue) Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions labels Apr 23, 2024
@PatrikLundell
Copy link
Contributor

What do you mean by paint meta data? My interest stems from the issue of "terrain" completely overwriting information about the actual terrain, and thus making it impossible to restore the actual terrain when the "terrain" is removed (you have to hard code some best guess).
By the way, if you find some way to associate paint info to terrain, I assume the logic would also apply to carpets?

@Procyonae
Copy link
Contributor Author

Procyonae commented Apr 24, 2024

Basically I have a local branch that makes it so piant can be applied to any terrain with a PAINTABLE flag (and consequently removed by a chipper still) without changing the id of the terrain itself, just the visual stuff it displays. For mapgen I think I'm looking at doing it as a "colour": { "!": [ "blue" ] } type thing but autogenerated ids might make sense too (so t_not_painted_blue). I just haven't looked into how to add a new field to track it in savefiles yet and am mildly scared of breaking everyone's saves.
I also needed
It's been a minute since I looked at it but I imagine making it handle furniture too wouldn't be that bad it'd just need to prompt you in the case of both a PAINTABLE terrain and furniture on one tile.
Idk how carpets should be handled tho.

@I-am-Erk
Copy link
Member

Can we also inherit colour? I see no reason to have to repeat it every time.

@Procyonae
Copy link
Contributor Author

Can we also inherit colour? I see no reason to have to repeat it every time.

Yes?

@Procyonae
Copy link
Contributor Author

Procyonae commented Apr 26, 2024

Ok going through all our ter/furn in this PR is enormous scope creep I think I'll dial this back to just C++ and removing redundant JSON stuff and do abstract/copy-from stuff in seperate follow-up PRs

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [Markdown] Markdown issues and PRs labels Apr 30, 2024
@github-actions github-actions bot added Mods Issues related to mods or modding Bionics CBM (Compact Bionic Modules) labels Apr 30, 2024
@github-actions github-actions bot added Mods: TropiCataclysm 🌴 Having to do with the tropical region mod for DDA. Mods: Xedra Evolved Anything to do with Xedra Evolved Mods: Mind Over Matter Mods: Sky Island Anything about the Sky Island mod labels Apr 30, 2024
@github-actions github-actions bot added Missions Quests and missions Crafting / Construction / Recipes Includes: Uncrafting / Disassembling labels May 29, 2024
@Procyonae
Copy link
Contributor Author

Ok the changes to bash and deconstruct are getting a bit much I think I'm going to scale back the PR a lil by reverting them and then just finalising what I have and then I'll attack them separately later

This reverts commit 11cdd92.

Revert "Deduplicate a couple of bash checks in lieu of making it optional"

This reverts commit af2417c.

Revert "Use std::optional for deconstruct"

This reverts commit 09dad60.

Revert "Less ugly variantless way"

This reverts commit 11fbb4a.

Revert "Slightly less gross but still std::variant"

This reverts commit 88eae1e.

Revert "std::variant works but I definately shouldn't be using it here"

This reverts commit b8130a5.

Revert "Somehow this is worse"

This reverts commit aaceacf.

Revert "Attempting to split map_bash_info"

This reverts commit bf733b1.
@Night-Pryanik
Copy link
Contributor

Closing as stale. If you wish to continue working on this, ping me to reopen.

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 Bionics CBM (Compact Bionic Modules) <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Crafting / Construction / Recipes Includes: Uncrafting / Disassembling <Documentation> Design documents, internal info, guides and help. Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display [Markdown] Markdown issues and PRs Missions Quests and missions Mods: Aftershock Anything to do with the Aftershock mod Mods: Defense Mode Anything to do with the Defense Mode mod Mods: Dinomod Anything to do with the Dinoclysm mod (DinoMod) Mods: Magiclysm Anything to do with the Magiclysm mod Mods: Mind Over Matter Mods: No Hope Relating to the mod No Hope Mods: Sky Island Anything about the Sky Island mod Mods: TropiCataclysm 🌴 Having to do with the tropical region mod for DDA. Mods: Xedra Evolved Anything to do with Xedra Evolved Mods Issues related to mods or modding Monsters Monsters both friendly and unfriendly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inheritance bug
4 participants