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.

Sign up for GitHub

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

Jump to bottom

Doc: Add labels to landscape grid description. #8452

Merged
merged 1 commit into from
Feb 14, 2021
Merged

Doc: Add labels to landscape grid description. #8452

merged 1 commit into from
Feb 14, 2021
+184 −241

Conversation

J0anJosep
Copy link
Contributor

@J0anJosep J0anJosep commented Dec 27, 2020 •
edited
Loading

Motivation / Problem

Current landscape_grid.html only tells which bits are used. If you want to know their meaning, you should look up their meaning in landscape.html.

I propose adding labels to the bits and some other things. But I won't mind if changes are rejected.

Description

The changes in landscape_grid.html include:

1.- Hovering over each bit shows a label describing its meaning.
2.- Some old "inherit" are changed with rowspans, trying to make it easier to know which tile class belong to and common meanings among "tile classes".
3.- Added the same header at the end of the table, as table is big and sometimes it is difficult to see the header when trying to read information from the end of the table.
4.- As some adjacent bits have different meanings, I separated them with a space. The right alignment of the bits sometimes is lost (it is difficult to keep them aligned).
5.- Added some more bit classes which also help in understanding the meaning of the bits.

Limitations

Bits are not always correctly aligned. Changes and colours are debatable. But I still think the grid is more helpful with these changes.
The columns "type" and "height" classes could be omitted or moved to the right side of the table, as they are common for all tile classes.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@LordAro
Copy link
Member

LordAro commented Dec 27, 2020

Sounds interesting! The html docs have needed some work for a while. Can you post a screenshot of the changes? Would save me checking out your branch ;)

@stormcone
Copy link
Contributor

@LordAro: http://htmlpreview.github.io/?https://github.com/OpenTTD/OpenTTD/blob/d6aa9fe24512ebac932efa668ba57290bee913fe/docs/landscape_grid.html

LordAro
LordAro previously requested changes Jan 2, 2021
View reviewed changes
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

Just a few things noticed while scrolling through - there could be a few others that i've missed, do check them

@J0anJosep J0anJosep force-pushed the LandscapeGridLabels branch from d6aa9fe to 11f298c Compare January 2, 2021 21:03
@frosch123
Copy link
Member

What is the difference between these two?

  • bit is used, but could be freed if needed
  • bit is accessed, but does not really have a meaning (e.g. owner of clear land is always OWNER_NONE)

@frosch123
Copy link
Member

The colors are nice, but using a,b,c,d,e for different types of "used" would be easier, and also allows searching.

@J0anJosep
Copy link
Contributor Author

What is the difference between these two?

* bit is used, but could be freed if needed

* bit is accessed, but does not really have a meaning (e.g. owner of clear land is always OWNER_NONE)

It is the same (I will update the PR). A bit which has no meaning could be freeded or used for other purposes. But for bit alignment mostly, this isn't done. If there was such a need for freeing the bits because there are no other free bits available, these "usable" bits could be considered. That's why I marked with a different type.

@J0anJosep
Copy link
Contributor Author

The colors are nice, but using a,b,c,d,e for different types of "used" would be easier, and also allows searching.

I don't understand the suggestion. Do you mean naming the classes used-a, used-b,...? If that's the case, you can't search for classes names in an html document except when editing the html code.

@frosch123
Copy link
Member

frosch123 commented Jan 3, 2021 •
edited
Loading

I meant replacing the "X" with different letters.
With so many colours I have difficulties to match the color of one "X" with the legend on the other side of the screen, with lots of colorful stuff inbetween.

@J0anJosep
Copy link
Contributor Author

I meant replacing the "X" with different letters.
With so many colours I have difficulties to match the color of one "X" with the legend on the other side of the screen, with lots of colorful stuff inbetween.

I have used X for a bit with undetermined value, and 1 or O for those bits whose value is known. I am not sure using other letters for different cases will help.

I will put the legend closer to the grid on the next update of the PR.

@J0anJosep J0anJosep force-pushed the LandscapeGridLabels branch 2 times, most recently from 394a82c to 93f55e4 Compare January 3, 2021 20:21
@J0anJosep
Copy link
Contributor Author

J0anJosep commented Jan 3, 2021 •
edited
Loading

Preview:
https://htmlpreview.github.io/?https://github.com/OpenTTD/OpenTTD/blob/946a8a109d839883323852a5e75cd8e2eb82ddf7/docs/landscape_grid.html

I aligned more bits.

@J0anJosep J0anJosep force-pushed the LandscapeGridLabels branch from 93f55e4 to cdd5297 Compare January 3, 2021 20:32
@J0anJosep J0anJosep force-pushed the LandscapeGridLabels branch from cdd5297 to 946a8a1 Compare January 3, 2021 20:42
@LordAro LordAro dismissed their stale review January 4, 2021 09:07

dismiss

LordAro
LordAro approved these changes Feb 13, 2021
View reviewed changes
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

LGTM, but I will leave merging to someone more familiar with the document

@frosch123 frosch123 merged commit a18188a into OpenTTD:master Feb 14, 2021
@J0anJosep J0anJosep deleted the LandscapeGridLabels branch November 22, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LordAro LordAro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@J0anJosep @LordAro @stormcone @frosch123