PETSCII background color issue

[bart-d]

Thanks for adding the PETSCII fonts. However there is a limitation with PETSCII that should probably be taken into account. PETSCII only allows for one background color to be chosen per image. The current implementation allows for 16 background colors which is with pure PETSCII impossible.

Each 8 x 8 pixel character location can contain 2 different colors: 1 color is defined per character location, and 1 color is defined for the entire screen background. All 16 colors can be used.

https://www.c64-wiki.com/wiki/Standard_Character_Mode

Would it be possible to have only one background color chosen when using any of the C64 fonts?

[andyherbert]

Doesn't it also have an inverse mode? Truth be told I didn't think about the implementation much, and didn't really see this as a real alternative to Petmate et al.

[bart-d]

it does have an 'invert' mode but that's already covered within the font, the upper half is inverted:

It does not give your an additional background color, it's just the glyphs inverted.

While I understand it's not intended to be a petmate alternative, it might be worthwhile not let the user make an accidential mistake by making an 'impossible combination'. Shouldn't be too hard imho to change the background color of the entire image whenever a new background color is selected for just the C64 fonts.

Note that technically speaking 4 background colors could be possible in "extended background mode", but that also limits the choice of characters to the first 64. Which is almost nothing but text and numbers. Not very useful for creating art. So I wouldn't go that far into getting it right. Single background color would suffice here imho.

[bart-d]

tested f8323339a74e2ba9a41e5c8a7f31b4ab56625b17 and found the background color to be set correctly now, thx! Maybe it's useful to add a PETSCII guide now, I made a PR to add a 40x25 guide.

i also looked at the c64 palette as it seems slightly off, the closest i could get was:

const c64_black = {r: 0, g: 0, b: 0};
const c64_white = {r: 63, g: 63, b: 63};
const c64_red = {r: 32, g: 13, b: 14}; 
const c64_cyan = {r: 29, g: 51, b: 50}; 
const c64_violet = {r: 35, g: 15, b: 37}; 
const c64_green = {r: 21, g: 43, b: 19}; 
const c64_blue = {r: 12, g: 11, b: 38}; 
const c64_yellow = {r: 59, g: 60, b: 28}; 
const c64_orange = {r: 35, g: 20, b: 10}; 
const c64_brown = {r: 21, g: 14, b: 0}; 
const c64_light_red = {r: 48, g: 27, b: 28};
const c64_dark_grey = {r: 19, g: 19, b: 18};  
const c64_grey = {r: 31, g: 31, b: 31}; 
const c64_light_green = {r: 42, g: 63, b: 39};
const c64_light_blue = {r: 26, g: 27, b: 58}; 
const c64_light_grey = {r: 44, g: 44, b: 44}; 

I'm a bit puzzled as to why there's a conversion from 6-bit to 8-bit, working this way it can't possibly ever be 100% correct. https://www.pepto.de/projects/colorvic/

[andyherbert]

It's to work with the XBIN file format which only has a provision for 64 levels for red, green, and blue. The values are stored as 8 bit bytes, so using one of the 3 unused bits in the header could serve as a switch to enable 8 bit per channel, but extending the file format in this way would probably never happen. As far as I understand it there is no absolute colour values for the C64, but only an approximation, and even if there were, I'd expect colours to appear differently on today's colour-calibrated displays.

[andyherbert]

Also, what is appveyor?

[bart-d]

Also, what is appveyor?

it's an CI/CD tool, was meant to to automatic builds, but didn't render the expect result. I asked @christianvozar a couple of times to remove already, i hope tagging him here helps ;)

[andyherbert]

I do intend to push the proposed palette changes at some point, but I'm getting a 'At least 1 approving review is required by reviewers with write access.' error when attempting to push my commit. I assume this has something to do with the tool raising concerns over a merge operation, but git is a powerful tool that I don't really understand outside the context of a single-user repo.

[christianvozar]

@bart-d I'll take care of appveyor today. Tagging me here did help. 😃

@andyherbert I have enabled protection of the master branch because we are now seeing multiple coders commit to the codebase and we want to avoid merge conflicts and issues. This is standard practice for 1+n contributors to a project. Essentially, all work should be done in a branch and pushed up to the repo in that branch. A PR is created from that branch and approved by someone who isn't the contributor and then it is merged. This slows down simply commits to the master branch but gives a workflow that prevents complications from competing commits, keeps quality high, etc.

We can be explicit with who has the ability to approve a merge (this could be kept to just me, burps and Andy for now) which allows anyone in the open-source world to contribute and us to gatekeeper what goes in.

[christianvozar]

@bart-d appveyor removed. I like the idea of a CI/CD that pushes compiled binaries/packages to the releases portion of GitHub so electron builds are automatic. Travis? Github actions?

[bart-d]

is that a sensible thing to do in a repo where 99,9% of the code is/was created by one single person? i can see the necessity with multiple committers, but so far there are only 1,01 ;)

[christianvozar]

@bart-d I think it's sensible to have two sets of eyes on any code merged into master on a product that people use, yeah. The process is super simple as it is, in order to merge your code in someone other than the commiter has to click "approve".

[andyherbert]

Yeah, I vote to drop it too. I thought it wouldn't matter but it's trivial to roll back changes anyway, and I would prefer people actually try the changes in ways that might break the app rather than try and reason about the code.

[andyherbert]

@christianvozar

[bart-d]

@christianvozar

[christianvozar]

@andyherbert @bart-d I've kept the branch protection rule but allowed you two specifically to over-ride this rule. If you could please test it out for me with an override that'd be cool. I think protecting the master branch is just good hygiene but understand your desire to keep velocity with trusted actors, so you two can override.

This also means that coders outside this small group can submit PR's and be reviewed and approved for merges if any of us three want to accept the PR.

[andyherbert]

I have literally no idea how to initiate an override. Trying git push --force doesn't work?

[christianvozar]

Oh I see you aren't submitting pull requests at all, just pushing straight to master.

It's a filthy habit but I'll just remove the protection on master.

[andyherbert]

Thanks.