Rewrite BCPD description of the colour data format - #662
Conversation
|
I suggest we add an example: |
|
That's actually a pretty good idea! A visual representation of how the encoding is transformed is very helpful. What I would like, actually, is to add a third layer, which shows how the colour is converted into RGB555 format, and then shows the two bytes being swapped in memory. Is the above image available in SVG form? I'd like to tweak it to propose some further improvements. (Perhaps we can add that in a follow-up PR, but still.) |
ISSOtm
left a comment
ISSOtm
left a comment
There was a problem hiding this comment.
Thank you for the wording improvements!
|
rgb555_encoding(1)(1).drawio |
|
Are these two distinct proposals? Or are they two distinct pictures that are both to be added? For my design, I actually like my "ATA/ATAPI cable" style because it is immediately clear which and how many bits are involved. |
|
I like the cable design as well @nummacway but I couldn't find a way to reproduce it in draw.io (which keeps the diagrams in a 'editable' project XML files so it's a preferred format here) :/ How was your diagram drawn? |
|
Didn't know draw.io was preferred. |
|
This is only the top part of the original file. For those who don't want to open drawio, here's a screenshot: |
|
I've started transcribing the design as a themable SVG (since no matter how much I try, I can't edit SVGs in graphical editors ^^') I'll host a rendered version online when I've gotten parity, though I also have design changes to suggest. In particular, this uses Pan Docs' font, which has different glyph widths, thus we may consider tweaking the font size? (Though I'm also thinking of swapping the values of the red and green components, simply to balance the amount of characters? xD) 
(Disregard the misaligned digits, I haven't bothered fixing those yet.) One change I've made is that the “ATA ribbons” are drawn behind the borders, which I'm not fully sure about. One possible change is to squish the ribbons vertically by 1 pixel at each edge, so that the black edges still render there. Feel free to throw feedback! |
One layout idea I had was to separate the format conversion and the RAM encoding into two separate layers, so there'd be three total: the RGB8 colour, the RGB555 version (still with the “ribbon cables”, and then two arrows pointing out how the bytes are swapped in memory order, and also dotted lines going left and right to suggest that there are more memory cells around. |
|
I like the diagram, but it took me a little while to understand where you're coming from. So I don't think I'm really the target audience for the diagram but I can see the value in it because a lot of people would be coming into this more familiar with this flavour of 24-bit colour than any other. It's a good way to illustrate "what RGB555 means" on a technical level. I've got one issue with the diagram itself: the order of the ribbon layers. Could the blue ribbon be on a higher layer than the green one? (blue should occlude green) RGB --> R, G_low, G_high, B @ISSOtm @avivace thanks for the review and patches! |
|
The layer idea was: Small cables over wide cables. |
|
My current iteration of the diagrams can be viewed online at https://eldred.fr/pandocs+662/Palettes.html#lcd-color-palettes-cgb-only; criticism welcome. [EDIT]: Updated with the state of the PR as of #662 (comment). |
This comment was marked as resolved.
This comment was marked as resolved.
|
Since there have been no replies to my last comment in the last two weeks, I'm going to push my changes to this PR, and we'll keep working from there. Anyone opposed to this, please voice your opinion (now or after) and those changes will be reverted. |
1b7a344 to
f924874
Compare
|
I'm now personally happy with the state of this PR, but I'd like opinions from @quinnyo and @nummacway before merging. |
|
Also @DelayRGC, since she opened the original issue. |
| All background colors are initialized as white by the boot ROM, however it is a | ||
| good idea to initialize all colors yourself, e.g. if implementing | ||
| a soft-reset mechanic. | ||
| All background colors are initialized as white by the boot ROM, however it is a good idea to initialize all colors yourself, e.g. if implementing a soft-reset mechanic. |
There was a problem hiding this comment.
| All background colors are initialized as white by the boot ROM, however it is a good idea to initialize all colors yourself, e.g. if implementing a soft-reset mechanic. | |
| All background colors are initialized as white by the boot ROM, however it is a good idea to initialize all colors yourself, e.g. by implementing a soft-reset mechanic. |
e.g. if implementing ... doesn't sound good
There was a problem hiding this comment.
"by" changes the meaning of the sentence. Maybe "for" would work better?
There was a problem hiding this comment.
That would also change the meaning of the sentence. The point is that it's a good idea to set all palettes yourself if there's any chance that the init code may run more than once, such as if the game has a soft-reset mechanism.
There was a problem hiding this comment.
What do you think of "for a soft-reset mechanic" rather than "for/by/if implementing a soft-reset mechanic"?
There was a problem hiding this comment.
I like Isso's original proposal.
There was a problem hiding this comment.
Simpler games that only use e.g. a single colour palette generally don't bother initialising more than that, and I don't want to imply that doing so is wrong.
I have moved the tip to the introduction.
There was a problem hiding this comment.
Oh, that makes sense, for sure. I kept saying "all the colours" in my previous comment, I suppose just because the text does the same. It doesn't need to say "all".
I'm not sure what the intent/point of the tip is -- outside of the description of the system startup behaviour. Possibly:
- The system only initialises the palettes when it first starts up.
- The colours will all be white by default, so the software must set up at least one palette for any background tile(maps) to be visible.
- At least during the development process, you might want to initialise palettes that you're not using to something discernable, if only to assist in spotting errors/bugs.
- If you're relying on the palettes being all-white, don't forget to reset that as part of a soft-reset.
Just the fact:
| All background colors are initialized as white by the boot ROM, however it is a good idea to initialize all colors yourself, e.g. if implementing a soft-reset mechanic. | |
| All background colors are initialized as white by the boot ROM. |
The software's responsibility:
| All background colors are initialized as white by the boot ROM, however it is a good idea to initialize all colors yourself, e.g. if implementing a soft-reset mechanic. | |
| All background colors are initialized as white by the boot ROM, so the software must set up any palettes it uses in order to have background tilemaps appear as anything other than solid white. |
There was a problem hiding this comment.
"as anything other than solid white." misses the point.
There was a problem hiding this comment.
OK. Please explain it to me.
I'm not sure what the intent/point of the tip is -- outside of the description of the system startup behaviour.
There was a problem hiding this comment.
My intent was indeed point 4, but I suppose it doesn't belong in Pan Docs much. Stating the hardware fact only should be sufficient here; we may even move it outside of a tip, though I'll check out the render to make that judgement.
|
Hi, sorry I haven't been around, I'll be able to have a look at this in a day or so! |
|
Ping? |
|
Reviewing now, sorry! |
quinnyo
left a comment
quinnyo
left a comment
There was a problem hiding this comment.
It's pretty much there.
The structural change -- moving the explanation out from under the register headings -- is a very welcome one. Good move.
Also good to settle on one set of register names (and on the more sensible Index and Data ones).
The biggest issue I see is not really introduced by this change, but is made more apparent by it -- the lack of basic introductory information about the CGB colours/palettes.
[possible further work] The bit 15 discussion seems to suggest there might be value in adding something about reading/writing hexadecimal colour words. Specifically about the nybbles not being aligned with the R,G,B components and the difficulty in interpreting that.
| All background colors are initialized as white by the boot ROM, however it is a | ||
| good idea to initialize all colors yourself, e.g. if implementing | ||
| a soft-reset mechanic. | ||
| All background colors are initialized as white by the boot ROM, however it is a good idea to initialize all colors yourself, e.g. if implementing a soft-reset mechanic. |
There was a problem hiding this comment.
Yes, changing it to "by" would make it have a different, also undesirable/confusing meaning. But the "if implementing" thing is odd, and the whole point being made is a bit strange, actually:
When wouldn't it be a good idea to initialise all the colours yourself? If they are all white you can't see anything.
Regardless of the behaviour of the boot ROM, it's a "good idea" to initialise thesome palettes, because you want your palette/s to be the ones being used. At least I think so.
[Note: I don't actually know how true the boot ROM statement is, I assume someone checked.]
I know this behaviour is specific to the BG colours, but this "tip" (and the similar one for OB) could be moved up to the general explanation above -- it's really not specific to the register/s.
|
FWIW, my focus is currently on doing some drawing in order to take a break from programming, so I'll review quinn's contributions to the PR after some days. I'll keep it under two weeks though. |
5222203 to
7849661
Compare
|
Whoops, never mind the pushes, I fat-finger'd a push to the wrong remote ^^' |
- less ambiguous description of the colour data format - clarify meaning of endianness (byte order) - reverse the order of the bit table, so MSB appears first, matching others
Co-authored-by: Eldred Habert <me@eldred.fr>
Co-authored-by: Eldred Habert <me@eldred.fr>
Co-authored-by: "Janni K." <24881711+nummacway@users.noreply.github.com> Co-authored-by: Rangi42 <sylvie.oukaour+rangi42@gmail.com>
Those have been deprecated in newer `hardware.inc`, and really, the header names were hard to read with the duplicated names, especially the OBJ ones. Anyone being confused by old code should be able to deduce the alternate naming from their definitions in the accompanying `hardware.inc`, or they can look it up on older versions of Pan Docs, or they can ask and be explained. I nonetheless expect that the impact will be very low at this point.
Co-authored-by: Quinn <3379314+quinnyo@users.noreply.github.com>
Co-authored-by: Quinn <3379314+quinnyo@users.noreply.github.com>
Fix wildy incorrect arithmetic Co-authored-by: Eldred Habert <me@eldred.fr>
7849661 to
b3a4387
Compare
Co-authored-by: Quinn <3379314+quinnyo@users.noreply.github.com>
Co-authored-by: Quinn <3379314+quinnyo@users.noreply.github.com>
|
OK, this should be all of it. Anything else? |
|
Big thanks to everyone who's pushed through this! I think the result is an excellent improvement ^^ |
5 participants
This is a rewrite of the BCPD section to hopefully reduce possible confusion and make the colour data format less ambiguous.
Particularly:
Fixes #643 (I think)