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

Doc: Add labels to landscape grid description. #8452

Merged
merged 1 commit into from Feb 14, 2021

Conversation

@J0anJosep
Copy link
Contributor

@J0anJosep J0anJosep commented Dec 27, 2020

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 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 ;)

Copy link
Member

@LordAro LordAro left a comment

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

docs/landscape_grid.html Outdated Show resolved Hide resolved
docs/landscape_grid.html Outdated Show resolved Hide resolved
docs/landscape_grid.html Outdated Show resolved Hide resolved
@J0anJosep J0anJosep force-pushed the J0anJosep:LandscapeGridLabels branch from d6aa9fe to 11f298c Jan 2, 2021
@frosch123
Copy link
Member

@frosch123 frosch123 commented Jan 3, 2021

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

@frosch123 frosch123 commented Jan 3, 2021

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

@J0anJosep J0anJosep commented Jan 3, 2021

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

@J0anJosep J0anJosep commented Jan 3, 2021

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 frosch123 commented Jan 3, 2021

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

@J0anJosep J0anJosep commented Jan 3, 2021

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 J0anJosep:LandscapeGridLabels branch 2 times, most recently from 394a82c to 93f55e4 Jan 3, 2021
@J0anJosep J0anJosep force-pushed the J0anJosep:LandscapeGridLabels branch from 93f55e4 to cdd5297 Jan 3, 2021
@J0anJosep J0anJosep force-pushed the J0anJosep:LandscapeGridLabels branch from cdd5297 to 946a8a1 Jan 3, 2021
@LordAro LordAro dismissed their stale review Jan 4, 2021

dismiss

Copy link
Member

@LordAro LordAro left a comment

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

@frosch123 frosch123 merged commit a18188a into OpenTTD:master Feb 14, 2021
8 checks passed
8 checks passed
Emscripten
Details
Commit checker
Details
Check preview needs update Check preview needs update
Details
Linux (clang, clang++)
Details
Linux (gcc, g++)
Details
Mac OS (x64, x86_64)
Details
Windows (x86) Windows (x86)
Details
Windows (x64) Windows (x64)
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants