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: add construction of wind turbines on roofs #4463

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

Lamandus
Copy link
Contributor

@Lamandus Lamandus commented Apr 10, 2024

Purpose of change

This adds the possibility to construct wind turbines on roofs. It require (imaginary) bolts and metal frames for securing it up there.

Describe the solution

Describe alternatives you've considered

Testing

Additional context

grafik

Checklist

@github-actions github-actions bot added the JSON related to game datas in JSON format. label Apr 10, 2024
@Lamandus Lamandus requested a review from chaosvolt April 10, 2024 09:21
@scarf005 scarf005 self-requested a review April 10, 2024 10:06
@scarf005 scarf005 self-assigned this Apr 10, 2024
Copy link
Member

@chaosvolt chaosvolt left a comment

Choose a reason for hiding this comment

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

Uuuugh that looks hacky, you've got to specify multiple redundant construction entries for every possible variation of roofs and you didn't even get them all in the process. Plus this falls apart for any building that has outdoor floors instead of roof tiles.

@Lamandus
Copy link
Contributor Author

Uuuugh that looks hacky, you've got to specify multiple redundant construction entries for every possible variation of roofs and you didn't even get them all in the process. Plus this falls apart for any building that has outdoor floors instead of roof tiles.

which one did I miss? and yes, there is no good other way to deal with it, the hacky thing is something you can't circumvent. And there are rarely roofs with regular tiles. So that works out for now.

@Lamandus
Copy link
Contributor Author

Uuuugh that looks hacky, you've got to specify multiple redundant construction entries for every possible variation of roofs and you didn't even get them all in the process. Plus this falls apart for any building that has outdoor floors instead of roof tiles.

This is what happens if you use "copy-from":
image

There is no way to define an array of pre-terrains. If anyone can provide this kind of solution, I would use it, CV.

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.

"pre_flags": [ "DIGGABLE", "FLAT" ], // (Optional) Flags the terrain must have to be built on
"pre_terrain": "t_pit", // (Optional) Required terrain to build on

this PR adds many duplicate entries, maybe we could remove pre_terrain and replace it with pre_flags?

@Lamandus
Copy link
Contributor Author

Lamandus commented Apr 10, 2024

@scarf005

 {
    "type": "terrain",
    "id": "t_flat_roof",
    "name": "flat roof",
    "description": "A flat, gray section of rooftop.",
    "symbol": ".",
    "looks_like": "t_concrete",
    "color": "dark_gray",
    "move_cost": 2,
    "flags": [ "TRANSPARENT", "FLAT" ],
    "bash": { "str_min": 30, "str_max": 210, "sound": "crash!", "sound_fail": "whump!", "bash_below": true }
  },

the problem is, we don't have a "hard roof" flag. Because a roof tag isn't enough, you shouldn't place it on a thatched roof. Should I really start with a new flag just for this?

@scarf005
Copy link
Member

scarf005 commented Apr 10, 2024

Should I really start with a new flag just for this?

this sounds much better in the long-term, as new flags can be used to simplify other building requirement. current solution of duplicating entries aren't scalable.

or we could just accept anything FLAT is good enough and install windmill on vinyl roof

@Lamandus Lamandus requested a review from scarf005 April 10, 2024 15:28
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.

image

@scarf005 scarf005 merged commit 9867e87 into cataclysmbnteam:main Apr 10, 2024
10 checks passed
@Lamandus Lamandus deleted the windm branch April 10, 2024 16:25
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants