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

feat(content, port): port treetops and ability to climb trees from DDA #5167

Merged
merged 10 commits into from
Nov 15, 2024

Conversation

ethanmeade
Copy link
Contributor

@ethanmeade ethanmeade commented Aug 8, 2024

Checklist

Required

Optional

Purpose of change

Ports over the ability to climb trees from DDA as implemented here. This does not port over the mutation changes regarding that PR; it also does not include tiles for the treetops themselves.

Describe the solution

Cherry-picked the code and JSON changes from the DDA PR to this code-base. This includes the addition of four new "treetop" terrains to the terrain-roofs JSON file and modifications of the tree entries in the terrain-flora JSON file. It also implements two new terrain flags: SINGLE_SUPPORT, which causes the terrain to be destroyed if the support under it is removed, even if there are adjacent supports, and CLIMB_ADJACENT, which allows the player to climb the terrain while standing adjacent to it.

Describe alternatives you've considered

Rewriting the code from scratch instead of porting it over from DDA. Alternatively, implementing no changes.

Testing

  • Generate a world with new trees
  • Climb tree without throwing an exception / generating errors
  • Climb down from tree without generating errors
  • Fall off of tree
  • Cut down a tree and check that the treetop is destroyed with it
    • Cut down a tree with supporting walls nearby and confirm that treetop is still destroyed

Additional context

@github-actions github-actions bot added src changes related to source code. data labels Aug 8, 2024
@ethanmeade
Copy link
Contributor Author

Currently, the game generates the world but throws errors while doing so. Actually attempting to climb a tree results in a crash. Working on debugging it now.

src/game.cpp Outdated
@@ -10031,7 +10039,7 @@ void game::vertical_move( int movez, bool force, bool peeking )

const auto cost = map_funcs::climbing_cost( m, u.pos(), stairs );

if( !cost.has_value() ) {
if( !cost.has_value() && !adjacent_climb ) {
Copy link
Member

@scarf005 scarf005 Aug 8, 2024

Choose a reason for hiding this comment

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

issue

this part crashes the vertical move code.

  1. cost is of type const std::optional<int>, and attempting to get value when it's empty throws exception. you need to check it does has value using cost.has_value below.
  2. originally, it made an early return if cost was empty in the below code block.
  3. however, in the new condition, the code block will not get executed when cost.has_value() == false and adjacent_climb == true
  4. which means it skips cost validation in https://github.com/ethanmeade/cataclysm-bn-local/blob/25b13fdb0aac0cb6d3a9ffc48ea6509198ab2acc/src/game.cpp#L10077 , hence the crash

possible solution

make sure cost is validated before getting evaluated.

@ethanmeade
Copy link
Contributor Author

ethanmeade commented Aug 8, 2024

Changed the trees to use the current CLIMBABLE terrain flag rather than the ported-over CLIMBABLE_ADJACENT terrain flag. Now it no longer crashes! Some issues do remain though: explicitly cutting a tree down does not remove the treetop, and when the treetop is removed it leaves behind an ASCII air tile rather than the normal graphical tile.

I'm not sure how difficult the trees should be to climb / move between, but I'm open to discussions on how that should be adjusted.

Edit: I most likely remove all references to the CLIMBABLE_ADJACENT tag in a future commit before taking this out of draft, assuming no reason for using it comes up during this PR.

@ethanmeade
Copy link
Contributor Author

So now treetops collapse whenever their parent tree is cutdown, but adjacent treetops also seem to collapse upon this happening, even if their parent trees are out of the way of the felled tree.

Amusingly enough, this behavior appears to also occur in DDA. Nevertheless, it doesn't seem that desirable, so I'm looking into preventing it.

Ethan Meade and others added 2 commits August 8, 2024 17:10
Collapses can no longer collapse roof's that are supported by walls. This means that roof collapse propagation will be contained within rooms. unless the walls are given `COLLAPSES` as a flag.

Makes `SINGLE_SUPPORT` flag unnecessary, the only edge case possible if that chopping down a tree will propagate collapse to any unsupported roofs nearby, which is easily explained by the tree hitting things on the way down and thus may be considered expected behaviour.
Copy link
Collaborator

@KheirFerrum KheirFerrum left a comment

Choose a reason for hiding this comment

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

SINGLE_SUPPORT flag is unnecessary, all behaviour is functionally identical with and without it. Roof's will still randomly collapse with changes made. Previous commit I did has removed the ability for roofs to collapse if they are supported by walls underneath. It also explains the edge case with this that wouldn't be resolved by SINGLE_SUPPORT as it is now but also doesn't actually need to be "fixed" in my opinion.

Please remove SINGLE_SUPPORT in light of this to prevent code from becoming more complex for no benefit.

Loading game also causes all treetops to trip JSON checks for bash_below, please append a "bash" field to your treetops. I suggest using the glass or paper roof as the basis of str_max and str_min

Finally, please make sure you remove flags that are no longer being used. CLIMBABLE_ADJACENT seems to have been deprecated by you for CLIMBABLE, so make sure to remove that from mapdata.h

@scarf005 scarf005 added JSON related to game datas in JSON format. and removed data labels Aug 28, 2024
Given that the PR OP doesn't seem to be around at the moment and they are fairly simple fixes.
Moves "roof" before "flag" on all trees

Removed unused giant fern treetop.
Copy link
Collaborator

@KheirFerrum KheirFerrum left a comment

Choose a reason for hiding this comment

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

Made a number of changes to the code and json. Should be ready for merging.
There's an unrelated issue where collapsed rooves form open air that displays as a placeholder instead of being transparent, but that bug is out of scope for this PR.

Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

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

@chaosvolt could you playtest this once more?

@scarf005 scarf005 marked this pull request as ready for review November 15, 2024 00:41
@ethanmeade
Copy link
Contributor Author

Sorry about not contributing further, got sidetracked and forgot about this. Let me know if there's anything I can do to help - although it looks like its already been handled outside of the play testing

Copy link
Collaborator

@RoyalFox2140 RoyalFox2140 left a comment

Choose a reason for hiding this comment

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

Compiled. Load Tested.

Climbing lone and multiple treetops works, smashing wood chopping and trees colliding into each other on wood chopping all works as intended.

Roof collapse on wall and floor destruction works as it used to even though this still means its inconsistent and allows for islands. Everything works and roof collapse sucking is out of scope.

@KheirFerrum
Copy link
Collaborator

KheirFerrum commented Nov 15, 2024

Sorry about not contributing further, got sidetracked and forgot about this. Let me know if there's anything I can do to help - although it looks like its already been handled outside of the play testing

No issue, it's mostly done now, not a lot of the C++ side remains from DDA's implementation, and the JSON side has seen extensive modification, but hell, it's not the first time we've had PRs end up being loosely inspired.

I mostly only looked at it since someone raised it in dev chat and I had a small amount of time on my hands

@scarf005 scarf005 merged commit 71e6f0c into cataclysmbnteam:main Nov 15, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON related to game datas in JSON format. src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants