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

Deduplicate "Can be cut down with the right tools" in description of trees #55207

Closed
NetSysFire opened this issue Feb 8, 2022 · 13 comments · Fixed by #55293
Closed

Deduplicate "Can be cut down with the right tools" in description of trees #55207

NetSysFire opened this issue Feb 8, 2022 · 13 comments · Fixed by #55293
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. <Suggestion / Discussion> Talk it out before implementing

Comments

@NetSysFire
Copy link
Member

Is your feature request related to a problem? Please describe.

It is annoying to have every tree description say "It can be cut down with the right tools." at the end.

Solution you would like.

Chopping down a tree works via the TREE flag. If the flag is present, farlooking the tree should append a "It can be cut down with the right tools" like you have with items.

This may be a good first issue but I do not know enough about the internals to say that for sure. Since this is not an item but rather something you inspect from a distance, this may be a bit more tricky.

Describe alternatives you have considered.

No response

Additional context

No response

@NetSysFire NetSysFire added <Suggestion / Discussion> Talk it out before implementing [C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. labels Feb 8, 2022
@LeahLuong
Copy link

Prob wouldn't hurt to simply remove it from the description. It's not like felling isn't 1 of the oldest activities known to humanity. It's also been a feature of sandbox video games since pterodactyls flew the skies.

@haveric
Copy link
Contributor

haveric commented Feb 11, 2022

I can implement this, but I need some more info on how you would like this to display. Should it append to the description similar to how it looks now or should it be on its own line before/after the other information?

@NetSysFire
Copy link
Member Author

Some objects already have info such as this:

image

I would put it on its own line with the other information. See what looks best, it might be good to place it above the "Cover" and other properties.

@haveric
Copy link
Contributor

haveric commented Feb 11, 2022

I've got a couple options that can be implemented. This has both the description and a Cuttable tag added.
image

Alternative positioning with modified text:
image

@NetSysFire
Copy link
Member Author

Nice! Regardless of the actual choice, please color "cut down" green. Right now, I would go for the second one since afaik all the properties of items and terrain are described more neutral and not in first person. Thanks to your POC screenshots I can isolate the issue better and it looks more "natural" when placed with the other properties.

The only other thing I could imagine right now is the duplication in "Cuttable" since "Can be cut down with the right tools" already covers that. Or vice-versa, but this becomes bike-shedding now.

@haveric
Copy link
Contributor

haveric commented Feb 11, 2022

Yeah, I threw both the text and the Cuttable into the same screenshots to avoid needing to make more screenshots. I think I'm going to remove the Cuttable, but it'll take me a bit to figure out how to colorize cut down correctly in this context.

@haveric
Copy link
Contributor

haveric commented Feb 11, 2022

If this looks good, I'll create the PR to get this added:
image

@NetSysFire
Copy link
Member Author

Please do submit a PR

@LeahLuong
Copy link

There should be a "the" preceding "New England" & a comma after "closely" in the 2nd sentence.

@haveric
Copy link
Contributor

haveric commented Feb 11, 2022

There should be a "the" preceding "New England" & a comma after "closely" in the 2nd sentence.

That's unrelated to this issue. There's likely more issues like this across all of the trees. To clarify, that is going to require a much bigger audit that is outside the scope of this request and should be its own issue.

@NetSysFire
Copy link
Member Author

Yeah, this is not a "technical" issue relating to code, just manual and boring grammar checking and therefore out of scope here.

Just creating an issue for each case of faulty grammar you find is not helpful though since fixing it is faster than reporting it. If you are looking for a good first thing to start contributing to, you just found it.

@LeahLuong
Copy link

Well, excuse me, I simply thought if they were going to be editing the text in this file it wouldn't be a big deal to correct the grammar errors @ the same time.

@haveric
Copy link
Contributor

haveric commented Feb 11, 2022

Well, excuse me, I simply thought if they were going to be editing the text in this file it wouldn't be a big deal to correct the grammar errors @ the same time.

I'm not sure why the attitude is needed as we're just trying to be helpful here. While it is easy to edit a little bit of text while making this change, it then becomes more difficult for a reviewer to review it as it's adding more complexity to the changes, especially as you would likely need to edit more than just this one tree to fix all of the issues with grammar related to this change.

TLDR: Because I edited the descriptions of all trees, I wouldn't be doing a grammar edit justice by only editing this one without spending the extra time needed to check the rest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. <Suggestion / Discussion> Talk it out before implementing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants