-
Notifications
You must be signed in to change notification settings - Fork 43
Added screen mode LCD #94
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
base: master
Are you sure you want to change the base?
Conversation
|
Isn't this better suited as separate mod? Also, if this is added, it probably would make more sense as separate node. |
It's a very small addition that I thought it would be OK for displaying things very quickly.
Digiscreens already exists, but it has a lower resolution (16x16 vs 56x48) and it is intended to be used with more screens to make a bigger one, while this implementation is intended for one single screen and quicker usage.
I also thought initially that making a new node made more sense, but then I wanted to try on giving more functionalities to the same screen and see how it would be. I personally think that it is not bad.
I'll take a look, thanks. |
SmallJoker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering how this is quite low in complexity and Lines of Code, I think this can be merged into digilines.
However, the PR needs rebasing and here's a few remarks...
| meta:set_string("message_type", type(msg)) | ||
|
|
||
| if msg ~= "" then | ||
| if type(msg) ~= string or msg ~= "" then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type(msg) ~= string is always true because type returns a string, and not a table (reference).
| end | ||
| pixels[i] = color | ||
| end | ||
| local png = minetest.encode_base64(minetest.encode_png(width, height, pixels)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core could be used instead of minetest. Both are the same, but since the engine was renamed, this notation seems dated. (all occurrences)
| return "#0000" | ||
| end | ||
|
|
||
| return string.format("#%X%X%X", num / 16^2, num / 16 % 16, num % 16 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the string.format? Couldn't we just use 0x notation in Lua?
|
|
||
| local number_to_color = function(num) | ||
| if type(num) ~= "number" or num >= 4096 or num < 0 then | ||
| return "#0000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of an error, it might make more sense to either:
- fall back to
0xF00(red = error) - or convert to 0x000 (black = error)
- or pass
tonumber(num) or 0to the numeric computation below and let the user see when it looks strange.
Also: What if nan is passed to this function?
| if message_type == "string" then | ||
| texture = generate_text_texture(create_lines(message)) | ||
| else | ||
| texture = generate_raster_texture(minetest.deserialize(message)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be documented somewhere. Otherwise no-one else would know about this raster feature.
I thought it would be interesting having the capability to display individual pixels on the LCD screen, so I made this feature.
It doesn't change the behavior when the LCD receives a string, but when it receives a table it will draw each individual pixel with 4096 possible colors (4 bits per channel).