Palette Control

[BombBloke]

Three gripes here, though I'm not entirely certain about the best resolution to any of them, so I figure a discussion is in order following on from #193 / #197.

1) Lack of a "term.resetPalette()", or somesuch. Once the palette of a given display is changed, the only way to switch it back via code is to do it manually. Having to store CC's "original" palette in user-scripts (eg so they can ensure correct rendering or clean up on exit) feels messy, especially since it's prone to change from version to version, hence I feel this would be worth adding.

Catch is that buffers (eg window) will need CC's default palette encoded into them.

Another existing workaround is obviously for the user to reboot their system, but that's a) messy b) not an option when the display is an external monitor (those require either replacing or a chunk reload).

2) colours.rgb8() converts three 0-1 values to a single RGB24 representation (eg 1,1,1 to 0xFFFFFF; you can alternatively have it perform the reverse, but since term.setPaletteColour() does that automatically it's somewhat redundant).

RGB8 conventionally refers to a mode where one byte is used per pixel (as opposed to intensity), so the function should hence be renamed - colours.rgb24() seems obvious, but maybe someone has a better title?

It could also stand to be range-limited, as it currently just checks whether the values it's passed are numeric. term.setPaletteColour() is also missing limit checks - this makes it appear to be broken at a glance, as passing in something like 200, 100, 50 (which users will do, though they're currently invalid) gets you 0x389CCE (equiv of 56, 156, 206: effectively inverting the values).

3) Use of 0-1 values to define intensities (versus 0-255) deserves re-raising; I've searched and searched but still cannot find a single implementation of an indexed display mode which does such a thing (and the non-indexed modes I can find certainly aren't from the era I've been assuming ComputerCraft to be themed upon).

If the goal is simply to "be different" then fine, but I believe coders and non-coders alike will be expecting standard triple-byte values (typically in the form of R,G,B), and I feel the current path will simply confuse people.

On the flip side, the only benefit I can see is marginally higher colour selection range - but we're talking about a 16 colour palette, so as benefits go it seems pretty slight.

[Lignum]

Lack of a "term.resetPalette()", or somesuch. Once the palette of a given display is changed, the only way to switch it back via code is to do it manually. Having to store CC's "original" palette in user-scripts (eg so they can ensure correct rendering or clean up on exit) feels messy, especially since it's prone to change from version to version, hence I feel this would be worth adding.

I actually did have a term.resetColours in my PR at one point which did exactly that. I didn't want to spam term with new functions, while still providing as much functionality as possible, so I replaced it with term.getPaletteColour, which is a lot more general, allowing users to implement their own term.resetPalette. It does feel messy, but is it worth adding yet another function to term?

RGB8 conventionally refers to a mode where one byte is used per pixel (as opposed to intensity), so the function should hence be renamed - colours.rgb24() seems obvious, but maybe someone has a better title?

A lot of the conventions used here, including the 0-1 range channels, come from OpenGL, where GL_RGB8 is a constant used to refer to this sort of encoding. Though, I agree, colours.rgb8 was added before term.setPaletteColour had that functionality, and now it's useless, we might as well remove it.

If the goal is simply to "be different" then fine, but I believe coders and non-coders alike will be expecting standard triple-byte values (typically in the form of R,G,B), and I feel the current path will simply confuse people.

The main idea was to abstract away the framebuffer's colour depth. While basically any framebuffer nowadays has a 24-bit depth, I still don't feel comfortable exposing that detail to ComputerCraft. Additionally, since we're passing these values to OpenGL either way, it just made sense.

On the flip side, the only benefit I can see is marginally higher colour selection range - but we're talking about a 16 colour palette, so as benefits go it seems pretty slight.

That's not a benefit either way, since the palette is encoded as a 32-bit integer array for networking purposes.

[BombBloke]

It does feel messy, but is it worth adding yet another function to term?

Do I look forward to adding all these functions to my own buffers? No, not really.

And if it weren't for monitors I wouldn't mind so much, but there's really no easy way to reset them as things stand, so I'm inclined to say "yes if there's no alternative solution"? getPaletteColour doesn't cut it, as there's no guarantee that the colours term.current() has when your script starts are the actual defaults, or at all suitable for any general purpose.

One such alternative which just came to mind is to let some other API deal with it. Something like colours.resetPalette( <termObject> ) for eg.

Though, I agree, colours.rgb8 was added before term.setPaletteColour had that functionality, and now it's useless, we might as well remove it.

While I doubt I'll be using it myself and won't mind terribly if it goes, I suspect some users might want to store colours within their code as single 24bit integers. It'd potentially be handy when comparing values obtained via term.getPaletteColour against those sorts of values, perhaps. Maybe.

The main idea was to abstract away the framebuffer's colour depth. While basically any framebuffer nowadays has a 24-bit depth, I still don't feel comfortable exposing that detail to ComputerCraft.

Surely using fractions to set depth, or making any references to OpenGL at all, is even more anachronistic? (I assume you're talking about anachronism here - please clarify if not. I can't see any value in "abstraction for the sake of abstraction", though - simplification, sure, but this isn't it).

Additionally, since we're passing these values to OpenGL either way, it just made sense.

I suspect #288 might've deviated things from that course a bit, though I confess I've no idea what impact it makes on performance.

[Lignum]

One such alternative which just came to mind is to let some other API deal with it. Something like colours.resetPalette( ) for eg.

That sounds like a reasonable compromise. Although it might fit into paintutils better? I'm not sure though.

Surely using fractions to set depth, or making any references to OpenGL at all, is even more anachronistic? (I assume you're talking about anachronism here - please clarify if not. I can't see any value in "abstraction for the sake of abstraction", though - simplification, sure, but this isn't it).

Yeah, I wasn't talking about anachronism. I figure the term abstraction was a bit ambiguous on my part, sorry. To rephrase, using 8-bit ints implies the colour depth of the client's window is 24-bit, which might not actually be the case for every client, which is also why OpenGL uses intensity instead. Additionally, this (again, implicitly) exposes technical details from behind the scenes, and ComputerCraft generally tends to avoid fourth wall breaks like that.

I suspect #288 might've deviated things from that course a bit, though I confess I've no idea what impact it makes on performance.

Basically none. Graphics processors nowadays can't deal with 64-bit floating points, so OpenGL just casts them back to 32-bit. Though, my point wasn't about performance, it was that it was just simple and intuitive to implement this way, so I just went along with it.

[BombBloke]

That sounds like a reasonable compromise. Although it might fit into paintutils better? I'm not sure though.

I considered paintutils, but I think that one may be better left to just "drawing stuff".

There's also the option of dropping it into term only (as opposed to "every terminal object"), though that might just be confusing. colours is probably best.

To rephrase, using 8-bit ints implies the colour depth of the client's window is 24-bit, which might not actually be the case for every client, which is also why OpenGL uses intensity instead. Additionally, this (again, implicitly) exposes technical details from behind the scenes, and ComputerCraft generally tends to avoid fourth wall breaks like that.

This seems contradictory - if not all client displays are at 24bit depth, then how does making all ComputerCraft terminals act like they are "break the fourth wall"?

[viluon]

I recently tried out one of the previews for CC 1.80, specifically to build on the palette functionality @Lignum has developed, and I must say that I agree with @BombBloke in nearly all of his points.

The new functionality was sadly a disappointment. I read through the discussion on https://github.com/dan200/ComputerCraft/pull/197, but only now was I forced to look at the issue purely from a user's perspective.

I wanted to set colours.grey to a custom value. The first thing I did was open up a colour picker (not unlike this one) to find the RGB values I was looking for. Please note that every widely used utility ever displays RGB as 8-bit integers, that is, integers from the 0-255 range. Surprise number one: CC does this differently. No problem I thought, let's just conform the manual - oh wait (surprise number two), this version of CC is still just a preview, meaning the wiki won't help me this time. Thankfully, I'm not a newbie and I knew all this, so my next stop was the relevant pull request. An inexperienced user would be lost at this point. It is worth noting that this was not an official build, and anyone using it would probably have the required knowledge to get over these problems. However, also note that the help program is no help at all in this case (would that be fixed for an official build? I doubt it), and the lack of available documentation is unacceptable. Moving on, the pull request ensured me that the variant of the function I'm using accepts normalised RGB values, I know how to deal with those. Finally! Surprise number three: term.setPaletteColour lacks an overload for using the colour's name (string value) as the first argument. (Admittedly, the majority of users would use colo?rs.* in the first place, but I was doing this programatically in a system build to possibly scale in the future. Since this is so easy to implement, I wonder why it isn't there already.)

Awesome. Changing the palette works. So now I would like to reset the palette back to the original one, after my program has done its job. Surprise number four, there's no easy way to do that (that's one of the few things the help program did tell me). Surprise number five (note that research beyond the available documentation was required to find this out, once again), term.getPaletteColour returns a single value, which is both not the common 0-255 RGB format everyone is used to and not the format I used for modifying the palette in the first place. Surprise number six, it seems that it doesn't even work properly. Setting colours.grey back to its pre-modification term.getPaletteColour value somehow breaks the background (which was supposed to be black, the text colour is colours.grey) (this is 1.80pr0 build 20, the same behaviour was observed on CCEmuX, so it's probably my misunderstanding of the API. Wouldn't be surprised if that were the case).

Let's sum up for a bit. I am now

• Puzzled by the new API design,
• Left with no easily available instructions on how to fix the behaviour,
• Afraid to proceed on my own, as so far palettes in ComputerCraft were completely counter-intuitive.

The current state of affairs goes radically against the principle of least astonishment. I've been a ComputerCrafter for years, and I find it ridiculous that after so much experience I find a couple of stdlib functions beyond my comprehension. I wouldn't like to blame @Lignum for how he implemented palettes. I know he comes from an OpenGL programmer background which clearly influenced his initial design plans for the feature. I greatly appreciate his devotion to improving the mod. But I think that the community pushed him to get over with frontend details as quickly as possible, as the conversation kept dragging on without much progress. I also think the code review prior to merging was lacking at best, and these issues should have been pointed out and fixed a long time ago.

Based on my observations and first-hand experience, I propose the following changes to the current API:

• 0-255 integers and 24 bit integers are by far the most widely used RGB formats. Stick to those instead of 0-1 ranges.
• The following overloads should be available for term.setColourPalette (and analogically for term.setColorPalette):
  • term.setColourPalette( name: string, r: int, g: int, b: int ) (r, g, and b must be from 0-255)
  • term.setColourPalette( name: string, rgb: int ) (rgb must be from 0-16777215)
  • term.setColourPalette( colour: int, r: int, g: int, b: int ) (r, g, and b must be from 0-255)
  • term.setColourPalette( colour: int, rgb: int ) (rgb must be from 0-16777215)
  • term.setColourPalette( mappings: table )
    • mappings is a table which satisfies the condition[1]
• The following overloads should be available for term.getColourPalette (and analogically for term.getColorPalette):
  • term.getColourPalette( name: string, use_24_bits: boolean ): ((r: int, b: int, g: int) | rgb: int)
  • term.getColourPalette( colour: int, use_24_bits: boolean ): ((r: int, b: int, g: int) | rgb: int)
    • If truthy, use_24_bits makes the function return a single 24 bit integer
  • term.getColourPalette(): table
    • This overload returns the full colour palette currently in use. The table maps colour numbers to 24-bit integers, which is the second most memory-efficient solution I can think of (the first being using the log( 2, colour ) values to make it a proper array, but that would lead to more processing and boilerplate code).
• It is worth noting that functions accepting a colour name must search in both the colors and the colours table, in this order. This also avoids the need to reimplement the functions for the second locale.
• Furthermore, to solve the problems outlined by this issue, I propose the following changes:
  • The name of colours.rgb8 is misleading. The function must be renamed or removed immediately. Since the aforementioned changes essentially solve the conversion issues, and converting between 0-255 and 0-1 ranges is straightforward, I'm in favour of removal.
  • term.resetPalette is absolutely necessary. Newly created buffers can query their parents for a colour palette with .getColourPalette(). This model is a compromise between term-only .resetPalette and using a different API for reverting to default colours (see [2]).

[1]: The condition

is met if the following code runs without errors:

for colour, rgb_data in pairs( mappings ) do
    term.setColourPalette( colour, type( rgb_data ) == "number" and rgb_data or unpack( rgb_data ) )
end

In other words, mappings entries can be of any of the overloads available for term.setColourPalette. Multiple arguments must be wrapped in an array.

[2]: Why I wouldn't like to see colours.resetPalette

colours and paintutils have each a distinct use case. colours manipulates colour values. It is most importantly a namespace for constants. Adding a colours.resetPalette function would break this convention. Suddenly, colours operates on a wider scope than it used to. Suddenly, one can't use term on its own, and colours is no longer just an API for handling numbers. The same goes for paintutils, a slightly higher-level graphics API designed to abstract away common drawing tasks and image manipulation. It has it in the name, "paint" - is that where you'd expect a function for mutating the state of a term? Maybe if it were playing hide and seek.

No.

When you find yourself looking for a function which mutates a term/window state, you look for it inside the object. Here we can see how CC implements one of the fundamentals of object-oriented programming, encapsulation, which keeps the actual data and implementation details of the object hidden from plain sight. The pre-palette term API had (and still has) its flaws regarding encapsulation, but putting .resetPalette somewhere else than into the object itself is counter-intuitive. Moreover, it would mean that the data required by .resetPalette must be opened up - after all, how would you reset to the default palette if you didn't know what its values actually were? We don't even have a way of reading data from a window (not that that would be a good thing, however). Opening this data up via methods would lead to even more "clutter" in the API, and exposing the table itself would mean anyone could tamper with it (__newindex can't stop everything), which is something the API design seemed to avoid fairly well so far.

The only meaningful place for .resetPalette is the buffer itself (and therefore also term).

[SquidDev]

Firstly @viluon, I have a great respect for you but could you tone it down a little? The previous post could be half the length if you left out the snarky comments and stuck to objective criticism. Great, now I'm doing snarky comments too :/.

The lack of available documentation is unacceptable.

I agree the builtin help files are lacking for the entirety of CC. It'd be nice to see much more detailed documentation, along with a decent browser (such as #212).

With regards to the wiki, I'm not really sure what a suitable solution would be. We've discussed this before (#271), but at that point I think we were assuming a release was imminent. It's been over three months since that issue, so it might be worth revisiting that. I know @BombBloke has talked about this, but I'm not sure whether that was about unofficial forks or not.

Surprise number three: term.setPaletteColour lacks an overload for using the colour's name (string value) as the first argument.

I'm not sure how this is a surprise? None of the term.set(Text|Background)Colour functions accept a string. In fact, seeing as green is no longer guaranteed to mean green, I'd argue allowing it to accept strings is counter-productive.

With regards to the whole 0-255 vs 0-1 debacle, I'm not sure you actually said anything new. We had a very long discussion about this before and, whilst I'd rather not witness that horror again it basically boiled down to "abstraction from platform" (0-1) and "what people expect" (0-255). I'd err on the side of the latter as CC generally tries to be as usable as possible. If we're going to re-live this (and, more importantly, convince Dan) then we're going to need some more compelling arguments.

With regards to your suggested overloads on getPaletteColour, I'm really not sure about the use_24_bits parameter. It seems a little ugly to have an boolean change the output of the function. You could have term.getPaletteColourPacked(colours.red) or something, but the term API is complicated enough as is. I'd personally prefer one or the other and doing colours.rbg8(term.getPaletteColour(colours.red)) to convert between them.

Which brings us smoothly onto colours.rgb8. My only complaint is that it does two things when it probably should only be doing one. I'd suggest having colours.packRBG(r, g, b):number and colours.unpackRGB(col):number,number,number. Whether this accepts/returns numbers in the range 0-1 or 0-255 is up to the aforementioned decisions for the term API.

I personally am not a great fan of any form of resetPalette. We've already seen various startup programs which change the user's palette, and restoring to the default on exit will cause problems with them. In the same way one shouldn't be doing term.redirect(term.native()) when exiting a program, one shouldn't be restoring to the default palette.

[viluon]

Thanks for the feedback @SquidDev, I did get carried away and I apologise for that. But I think the points still stand.

Docs need a reliable system, but that's probably better to discuss in another thread.

I apparently provided insufficient reasoning for the string colour name as an argument to palette functions. The scenario I found myself in required the use of a custom colour system, where the standard constant names would no longer hold their vanilla values. While this was probably quite a rare case, I suspect similar constructs will become more frequent as the palette system hits production. I was thinking a means of bypassing the colours API while retaining the usefulness of string names would come in handy (this was meant for palettes only, where iteratively changing a number of colours seems to be a pretty common task). Looking back now, the inconsistency alone such a change would introduce probably makes it a bad idea, even though it's a one-liner. It would, however, simplify the implementation of the proposed overload for term.setPaletteColour() (without arguments) and allow for passing tables such as { black = 0x000000; white = 0xffffff } to it.

As @BombBloke pointed out, 0-1 doesn't fit with ComputerCraft's retro style, and the platform abstraction point doesn't really make sense (I'm sorry if that's not what you meant @BombBloke, please correct me if that is the case). 0-255 is least astonishment. Even if that were the only point why 0-255 should be used instead, why would anyone choose 0-1? I'm not being sarcastic, I just really don't see a point. I doubt it's for the performance benefit when passing the values to OpenGL.

Regarding colours.rgb8: when passed a 24 bit integer, the function returns three 0-1 numbers. This has nothing to do with the 8-bit (0-255) RGB format. The name is misleading and must be changed. colours.(un)?packRGB seems like a good idea, and you nicely avoided the bit precision too.

I really think resetPalette is necessary. Its absence pushes users to ship their programs along with a full (default) palette, and hurts backwards-compatibility with anything pre-1.80. That being said, trying to maintain backwards compatibility with a preview seems like way too much to me. CC is known for being very conservative, but it can only go so far before such behaviour hurts the code base.

I'm really sorry for my wording, especially to @Lignum, the author of the changes. I didn't want to insult anybody, only voice my frustration at the current state of the APIs (which I, as you might have guessed, find unfortunate).

EDIT: Help/explanation regarding the end result of my palettising would be greatly appreciated. Is this (the linked screenshot) intended behaviour?

[BombBloke]

The reason I'd personally like to see a palette-resetter implemented outside of term is because I've coded a number of "virtual" terminals. I really don't want to have to hard-code ComputerCraft's default colour set into them all, especially since those colours may possibly vary from build to build! Indeed, I don't want to have to add another function to them at all!

While I agree that scripts shouldn't blindly reset their colours to default upon exit ("what they started with" is better, if they're going to reset at all), I disagree that they shouldn't have an easy way to ensure they're using the proper colours upon startup. Potential misuse of a tool doesn't seem a good reason to leave it out.

I have no idea what is meant by "the data required by colours.resetPalette must be opened up". If a function that accepts a term object as a parameter and resets its palette were to be placed within the colours API, and obtained its data through a table pointer localised to that API, then I see no practical method by which external code could tamper with it that couldn't also be applied to a version placed in term.

With regards to the wiki, I'm not really sure what a suitable solution would be. We've discussed this before (#271), but at that point I think we were assuming a release was imminent. It's been over three months since that issue, so it might be worth revisiting that. I know @BombBloke has talked about this, but I'm not sure whether that was about unofficial forks or not.

I was solely referring to unofficial forks there (with additional functions / peripherals / whatever that aren't in mainline). In regards to documenting the mainline builds, I agree it's worth revisiting.

My personal main beef against documenting now is the 0-1 / 0-255 thing (and to a lesser extent, the colours.rgb8 thing). Whether changed before release (and / or documentation, which amounts to the same thing) or not, I'd very much like to know that the behaviour won't change after - that'll lead to backwards compatibility issues, and I dislike that idea even moreso than I dislike the current scheme. Hence I do wish more people would throw their thoughts into this discussion. @dan200 especially, since it all hinges on him. To date we know nothing of his reasoning.

As @BombBloke pointed out, 0-1 doesn't fit with ComputerCraft's retro style, and the platform abstraction point doesn't really make sense (I'm sorry if that's not what you meant @BombBloke, please correct me if that is the case).

That's indeed what I meant. The chosen "abstraction method" is completely divorced from that of any palette-based format that I'm aware of, and to me it just highlights that we're running code through a "virtual machine" rather than a "virtual computer".

term.getPaletteColour returns a single value

No, it returns three values in the range of 0-1. Eg:

local myCol = {term.getPaletteColour(colours.white)}  --# Build a table of the three values.
term.setPaletteColour(colours.white, table.unpack(myCol))  --# Pass them back in verbatim.

[SquidDev]

The reason I'd personally like to see a palette-resetter implemented outside of term is because I've coded a number of "virtual" terminals.

Ahhh, that's a fair point. I wonder if something like term.nativePaletteColour(color) would be an alternative solution? That way to restore the palette you can do something like this:

for i = 0, 15 do term.setPaletteColour(2^i, term.nativePaletteColour(2^i)) end

It also gives you the ability to selectively restore colours, etc... If the palette functions accepted/returned tables then this might be a little easier, but that's out of the question sadly.

[BombBloke]

That wouldn't cause any "difficulties" (as such a function would only need to exist within the term API, as opposed to terminal objects), but it still makes more sense to me to put a function that returns the regular values for colours into the colours API.

[viluon]

I have no idea what is meant by "the data required by colours.resetPalette must be opened up"

I'm sorry @BombBloke, it seems I misunderstood your initial description of .resetPalette. You seem to assume that it sets the current palette to the default colours, while I expected it would reset to the colours the object started with (the initial palette being pulled from its parent). With my implementation, the user script would set its own palette and term.resetPalette() to get the original one back (hence the need for an inner data structure holding this version of the palette), but wouldn't actually have access to the default colours. For that, something like colours.default would be needed :confused: Your understanding is a lot more useful by comparison. (Still, a function for manipulating palettes logically belongs among the other ones. #433 implements native palette getters instead, which avoids the problem, but forces users to do the resetting themselves.)

Thanks for clarifying the function signature too, my script now works as intended.

Seeing as @Lignum already started working on the changes, and incorporated the best of the suggestions, I think #433 can close this issue when merged (and possible additional discussion can move to the PR's thread).