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

Tiles can now be modified by arbitrary tool qualities #12738

Closed
wants to merge 19 commits into from

Conversation

Mervill
Copy link
Member

@Mervill Mervill commented Nov 23, 2022

About the PR

This is a big refactor about Tiles. The focus is to make it so that Tiles can now define arbitrary tool qualities for modifying them. This replaces the existing and somewhat snowflake-y canCrowbar and canWirecutter pathways for modifying Tiles.

  • canCrowbar and canWirecutter on Tiles has been replaced with deconstructToolQualities which is a list that defines what tool qualities can cause a tile to switch to its baseTurf
  • LatticeCutting and TilePrying have been combined into ToolWorksWithTiles which allows an object that also has the Tool component to work on tiles.
  • There is now only one baseTurf per Tile instead of a list of turfs.
  • underPlating is now completely depreciated

This PR contains no gameplay changes. In the future I hope certain construction focused forks will let me rip up plating without having to resort to explosives.

@Mervill
Copy link
Member Author

Mervill commented Nov 23, 2022

Draft because big + tiles are important + sill making sure I got everything + ratio

@Mervill Mervill marked this pull request as ready for review January 5, 2023 18:49
@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Jan 5, 2023
@Mervill
Copy link
Member Author

Mervill commented Jan 5, 2023

ok so this actually dosen't have any merge conflicts two months later? how is that even possible...

@github-actions
Copy link
Contributor

github-actions bot commented Jan 7, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Jan 7, 2023
Copy link
Contributor

@mirrorcult mirrorcult left a comment

Choose a reason for hiding this comment

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

sorry for many conflicts now but this looks good outside of one thing

if (component.RequiresUnobstructed && tileRef.IsBlockedTurf(true))
return;

if (tileRef.TryDeconstructWithToolQualities(tool.Qualities, _mapManager, _tileDefinitionManager, EntityManager))
Copy link
Contributor

Choose a reason for hiding this comment

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

this function should just get moved to being in the same system, i doubt it will be called elsehwere? (also the other tileref methods in this same vein were removed in a diff PR)

@mirrorcult mirrorcult added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Jan 20, 2023
@metalgearsloth
Copy link
Contributor

@Mervill are you coming back to this

@metalgearsloth metalgearsloth added the S: Derelict Status: Abandoned, but may contain something that can be salvaged. label Feb 13, 2023
@20kdc
Copy link
Contributor

20kdc commented May 19, 2023

Right, so, of this PR, I've managed to get the following two parts merged:

  • baseTurf rather than baseTurfs
  • underPlating is now removed

Which leaves:

  • Replacing canCrowbar / canWirecutter with deconstructToolQualities
  • LatticeCutting and TilePrying into ToolWorksWithTiles

The problem is I'm not actually sure about this approach. Does it actually make sense to have a tool that has the qualities of a crowbar but doesn't work on tiles for some reason? If it does, what about the inverse?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Awaiting Changes Status: Changes are required before another review can happen S: Derelict Status: Abandoned, but may contain something that can be salvaged. S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants