john
/
McRogueFace
1
0
Fork 0
Code Issues 52 Pull Requests Projects 1 Releases Wiki Activity

Standardize Color Handling #11

New Issue
Closed
opened 2024-03-09 18:31:17 +00:00 by john · 7 comments
john commented 2024-03-09 18:31:17 +00:00
Owner
Copy Link

notes say:
15 refactor: standardize color handling: 3- or 4-tuple, 3 or 4 member lists, any mix of ints or floats (standardized to 0 - 255); or color object

This issue is for the method to take a PyObject* and return a correctly assembled mcrfpy.Color (maybe a sfml.Color later on, if there's a suitable SFML Python implementation that can be shipped with McRogueFace)

Once it's available, every widget that uses color (Frame, Caption, Grid, GridPoint) should have their versions replaced

notes say: 15 refactor: standardize color handling: 3- or 4-tuple, 3 or 4 member lists, any mix of ints or floats (standardized to 0 - 255); or color object This issue is for the method to take a PyObject* and return a correctly assembled mcrfpy.Color (maybe a sfml.Color later on, if there's a suitable SFML Python implementation that can be shipped with McRogueFace) Once it's available, every widget that uses color (Frame, Caption, Grid, GridPoint) should have their versions replaced
john added the
Alpha Release Requirement
Refactoring & Cleanup
Bugfix
Major Feature
 labels 2024-03-09 18:31:17 +00:00
john changed title from Fix Color Handling to Standardize Color Handling 2024-03-09 18:32:30 +00:00
john added this to the All Datatypes Behaving milestone 2024-03-13 12:59:20 +00:00
john added this to the The Datatype Overhaul project 2024-03-13 13:06:47 +00:00
john modified the milestone from All Datatypes Behaving to Alpha Release Targets 2024-03-22 02:42:39 +00:00
john modified the milestone from Alpha Release Targets to All Datatypes Behaving 2024-03-22 02:42:42 +00:00
john started working 2024-03-23 17:06:18 +00:00
john commented 2024-03-23 17:49:06 +00:00
Author
Owner
Copy Link

PyColor

This type has some other requirements that make it differ from PyTexture:

  • Textures are held by UIDrawables as a way to keep the Texture loaded (since sfml requires them to stay loaded). Colors internal to UIFrame's sf::RectangleShape or UICaption's sf::Text are not pointers - I could point to them, but I can't own them as shared pointers.
  • The current PyColorObject holds a shared pointer to a sf::Color, which makes the code pattern consistent, but is a pretty unremarkable

So what should a PyColor actually hold?

a pointer to a sf::Color, a weak_ptr to a UIDrawable, and an enum for the particular color it targets.

  • The weak_ptr works because UIDrawables are created in the Scene's std::shared_ptr<std::vector<std::shared_ptr<UIDrawable>>> ui_elements;
    therefore UIDrawable can inherit from enable_shared_from_this and always provide a shared_ptr to the UIDrawable
  • If the weak_ptr is invalid, the color is "dead", and can't modify or read the color's value.

There will also be some confusing behavior in terms of assignment -

grid_point.background_color = (255, 128, 64)
grid_point.background_color = sprite.highlight_color
  • we can control these assignments with setattr on the Python types of the objects. Those "color figure-outer" methods should be static methods in the new PyColor. After building a PyColor linked to grid_point's background_color attribute, we can call "set(args)" to use the pointer and update the value.
c = grid_point.background_color
c = (255, 128, 64)

This will never work, it simply overwrites the value of c. We can give c as much behavior as we want to make it convenient and have a link to the background color, but the = operator can't be overwritten.

c = grid_point.background_color
c.set(255, 128, 64)

This will work - the color pointer can be modified, which edits the grid point. If you held on to c longer than grid_point's lifetime, we can safely raise a Python exception.

Does the new PyColor need shared_from_this() ? Seemingly not - it doesn't load its own data into a shared_ptr to send as a Python object.

# PyColor This type has some other requirements that make it differ from PyTexture: * Textures are held by UIDrawables as a way to keep the Texture loaded (since sfml requires them to stay loaded). Colors internal to UIFrame's sf::RectangleShape or UICaption's sf::Text are not pointers - I could point to them, but I can't own them as shared pointers. * The current PyColorObject holds a shared pointer to a sf::Color, which makes the code pattern consistent, but is a pretty unremarkable So what should a PyColor actually hold? a pointer to a sf::Color, a weak_ptr to a UIDrawable, and an enum for the particular color it targets. * The weak_ptr works because UIDrawables are created in the Scene's `std::shared_ptr<std::vector<std::shared_ptr<UIDrawable>>> ui_elements;` therefore UIDrawable can inherit from `enable_shared_from_this` and always provide a `shared_ptr` to the UIDrawable * If the weak_ptr is invalid, the color is "dead", and can't modify or read the color's value. There will also be some confusing behavior in terms of assignment - ```python grid_point.background_color = (255, 128, 64) grid_point.background_color = sprite.highlight_color ``` - we can control these assignments with `setattr` on the Python types of the objects. Those "color figure-outer" methods should be static methods in the new `PyColor`. After building a `PyColor` linked to `grid_point`'s `background_color` attribute, we can call "`set(args)`" to use the pointer and update the value. ```python c = grid_point.background_color c = (255, 128, 64) ``` This will never work, it simply overwrites the value of `c`. We can give `c` as much behavior as we want to make it convenient and have a link to the background color, but the `=` operator can't be overwritten. ```python c = grid_point.background_color c.set(255, 128, 64) ``` This will work - the color pointer can be modified, which edits the grid point. If you held on to `c` longer than `grid_point`'s lifetime, we can safely raise a Python exception. Does the new PyColor need `shared_from_this()` ? Seemingly not - it doesn't load its own data into a shared_ptr to send as a Python object.
john stopped working 2024-03-23 17:49:11 +00:00
42 minutes 53 seconds
john started working 2024-03-24 00:28:19 +00:00
john commented 2024-03-24 03:06:00 +00:00
Author
Owner
Copy Link

Got a bit distracted, but I was having fun. I wanted to give colors readable names and also have an absurd number of plain text names that could be used.

Basically took CSS color codes, the XKCD color survey, and a wikipedia article on crayons' colors and made a list of 1,258 named colors. I initially wanted to do compile-time processing in a C++ file to generate a table including the values needed to make the "nearest name" test for a given RGB value just a single simple subtraction.

Yes, the whole point of this thing is to take any RGB value and give it a name.

>>> largest_dist = 0
>>> for c in all_colors:
    dist = rgbdist(c, c.nearest_neighbor)
    if dist > largest_dist: largest_dist = dist
    if dist < smallest_dist: smallest_dist = dist
    #print(f"{c.prefix}:{c.name} -> {c.nearest_neighbor.prefix}:{c.nearest_neighbor.name}\t{rgbdist(c, c.nearest_neighbor):.2f}")
    
>>> largest_dist
39.06404996924922
>>> smallest_dist
1.0

This is an unfinished feature, but the goal output for this script is a C++ file - it'll give the RGB code and color name in a single static array, sorted by palette priority then by "voronoi radius" (to reduce average lookup time) so that the first result in the list is the correct answer.

See 2cac6f03c6 for initial scribbles for the PyColor class; see 3728e5fcc8 for the color mapping code

Got a bit distracted, but I was having fun. I wanted to give colors readable names and also have an absurd number of plain text names that could be used. Basically took CSS color codes, the XKCD color survey, and a wikipedia article on crayons' colors and made a list of 1,258 named colors. I initially wanted to do compile-time processing in a C++ file to generate a table including the values needed to make the "nearest name" test for a given RGB value just a single simple subtraction. Yes, the whole point of this thing is to take any RGB value and give it a name. ``` >>> largest_dist = 0 >>> for c in all_colors: dist = rgbdist(c, c.nearest_neighbor) if dist > largest_dist: largest_dist = dist if dist < smallest_dist: smallest_dist = dist #print(f"{c.prefix}:{c.name} -> {c.nearest_neighbor.prefix}:{c.nearest_neighbor.name}\t{rgbdist(c, c.nearest_neighbor):.2f}") >>> largest_dist 39.06404996924922 >>> smallest_dist 1.0 ``` This is an unfinished feature, but the goal output for this script is a C++ file - it'll give the RGB code and color name in a single static array, sorted by palette priority then by "voronoi radius" (to reduce average lookup time) so that the first result in the list is the correct answer. See 2cac6f03c601de4591dbd8205418a1cbfe7e7e9f for initial scribbles for the PyColor class; see 3728e5fcc8bd745ef0268312a808fab6c82d7d91 for the color mapping code
john stopped working 2024-03-24 03:08:11 +00:00
2 hours 39 minutes
john added spent time 2024-03-25 01:22:28 +00:00
30 minutes
john commented 2024-03-27 03:12:52 +00:00
Author
Owner
Copy Link

Some quick thoughts on 13a4ddf41b

I'm honestly not feeling great about how this feature is going - the time spent is getting out of hand, and the PyColor class is huge. There are three modes to it. I have a sinking feeling that this thing is going to get gutted before the issue gets closed.

  • Dangling pointers are a real risk. The "self-owned" mode of the pointer is supposed to allocate and manage its own sf::Color* and delete it on cleanup. So new PyColorObjects / PyColors need to drop from SELF_OWNED to BORROWED so they don't free the reference they borrowed from. But if the original gets cleaned up first, it'll segfault the next time the child gets accessed. So this "self owned" method should never be used from Python, and there's no standalone color container functionality.

The main reason for all this bloat is because I wanted colors that were linked to a parent's property. So I built it around a pointer, but the truth is most UIDrawable objects use SFML getters/setters to change color values. So I tacked that on too - function pointers.

So to streamline this thing as I try to get it across the finish line:

  • Callbacks are the way to go. Most fill/outline colors need to be modified with methods. If something like a UIGrid wants to instantiate a sf::Color for tracking fill colors or Grid data, then we can pass a lambda.
  • This pointer nonsense needs to be gutted. Do I need to subclass PyColor, or just slap a sf::Color property onto it?
  • The _PyColorData struct didn't help the complexity of the class - it probably hurt it. I broke the pattern of the Python object storing a shared pointer to the C++ object, so now I'm doing witchcraft with static methods to create a PyColor using a PyColorObject's data contents. I think I can say for sure that I don't like how it looks right now.

almost 300 lines in PyColor's files - I bet it can be done nicely in 200 lines.

Some quick thoughts on 13a4ddf41b41dfc123a00468377b4f8fae0da845 I'm honestly not feeling great about how this feature is going - the time spent is getting out of hand, and the PyColor class is huge. There are three modes to it. I have a sinking feeling that this thing is going to get gutted before the issue gets closed. * Dangling pointers are a real risk. The "self-owned" mode of the pointer is supposed to allocate and manage its own `sf::Color*` and delete it on cleanup. So new `PyColorObject`s / `PyColor`s need to drop from `SELF_OWNED` to `BORROWED` so they don't free the reference they borrowed from. But if the original gets cleaned up first, it'll segfault the next time the child gets accessed. So this "self owned" method should never be used from Python, and there's no standalone color container functionality. The main reason for all this bloat is because I wanted colors that were linked to a parent's property. So I built it around a pointer, but the truth is most UIDrawable objects use SFML getters/setters to change color values. So I tacked that on too - function pointers. So to streamline this thing as I try to get it across the finish line: * Callbacks are the way to go. Most fill/outline colors need to be modified with methods. If something like a UIGrid wants to instantiate a sf::Color for tracking fill colors or Grid data, then we can pass a lambda. * This pointer nonsense needs to be gutted. Do I need to subclass PyColor, or just slap a sf::Color property onto it? * The `_PyColorData` struct didn't help the complexity of the class - it probably hurt it. I broke the pattern of the Python object storing a shared pointer to the C++ object, so now I'm doing witchcraft with static methods to create a `PyColor` using a `PyColorObject`'s data contents. I think I can say for sure that I don't like how it looks right now. almost 300 lines in PyColor's files - I bet it can be done nicely in 200 lines.
john added spent time 2024-03-27 03:13:03 +00:00
1 hour 30 minutes
john started working 2024-03-28 00:24:37 +00:00
john commented 2024-03-28 01:23:08 +00:00
Author
Owner
Copy Link

41509dfe96 - PyLinkedColor initial version

PyLinkedColor is my answer to the thoughts from last night. I'm going to un-refactor PyColor to have no frills, just a simple sf::Color instance to copy by value. The linked variant uses set() and get() to call functions.

Sure enough:

 wc -l PyLinkedColor*
 144 PyLinkedColor.cpp
  58 PyLinkedColor.h
 202 total

But I've hit an architectural snag:

/home/john/Development/McRogueFace/src/UI.h:617:41: error: ISO C++ forbids taking the address of a bound member function to form a pointer to member function.  Say ‘&sf::Text::setFillColor’ [-fpermissive]

I can't pass UIDrawable.some_sfml_member.getFillColor as the function pointer.

However, PyLinkedColor already requires a weak_ptr to its parent, and an integer index.

So the solution to color linking seems to require:

  • Virtual Methods on the UIDrawable base class, sf::Color getColor(int) and void setColor(int, sf::Color).
  • Static methods (probably in UIDrawable, for organization), to kick off the polymorphism. static void UIDrawable::getColor(UIDrawable&, int) and static sf::Color UIDrawable::setColor(UIDrawable&, int, sf::Color)
  • Derived method on all of the UIDrawable derived classes that interprets the int as some member and calls setColor or getColor.
  • It would be good if the table of int -> field member were somehow available to PyLinkedColor to mention what field it modifies in __repr__, but I'll probably settle for hand-copying it now.
41509dfe9640a67f924c5f843fe6bceb0cdb8f78 - PyLinkedColor initial version `PyLinkedColor` is my answer to the thoughts from last night. I'm going to un-refactor `PyColor` to have no frills, just a simple sf::Color instance to copy by value. The linked variant uses set() and get() to call functions. Sure enough: ``` wc -l PyLinkedColor* 144 PyLinkedColor.cpp 58 PyLinkedColor.h 202 total ``` But I've hit an architectural snag: ``` /home/john/Development/McRogueFace/src/UI.h:617:41: error: ISO C++ forbids taking the address of a bound member function to form a pointer to member function. Say ‘&sf::Text::setFillColor’ [-fpermissive] ``` I can't pass `UIDrawable.some_sfml_member.getFillColor` as the function pointer. However, PyLinkedColor already requires a weak_ptr to its parent, and an integer index. So the solution to color linking seems to require: * Virtual Methods on the UIDrawable base class, `sf::Color getColor(int)` and `void setColor(int, sf::Color)`. * Static methods (probably in UIDrawable, for organization), to kick off the polymorphism. `static void UIDrawable::getColor(UIDrawable&, int)` and `static sf::Color UIDrawable::setColor(UIDrawable&, int, sf::Color)` * Derived method on all of the UIDrawable derived classes that interprets the int as some member and calls setColor or getColor. * It would be good if the table of int -> field member were somehow available to PyLinkedColor to mention what field it modifies in `__repr__`, but I'll probably settle for hand-copying it now.
john stopped working 2024-03-28 01:23:21 +00:00
58 minutes 44 seconds
john commented 2024-03-29 00:57:30 +00:00
Author
Owner
Copy Link

Let's talk about 06e24a1b27

so I did not do what I said I would do last night. Instead of building a workaround for function pointers, I dusted off the technique I used to pull off animations, and upgrade PyLinkedColor to use std::function. I form a closure over the shared_ptr to the UIDrawable derived object, then wrap whatever get/set color code (for a property or a SFML member object method) to pass get and set to the linked color object.

Let's try and close this up:

  • PyLinkedColor properties: r, g, b, a, color
  • PyLinkedColor set(c) method
  • PyColor - just fix it entirely
  • Color names - I think I want to close the issue without solving this, and open a new one. It's such a fun feature, but it's not on the critical path.
Let's talk about 06e24a1b27c2f1ec520537f3a5b9b08d68d07829 so I did not do what I said I would do last night. Instead of building a workaround for function pointers, I dusted off the technique I used to pull off animations, and upgrade `PyLinkedColor` to use `std::function`. I form a closure over the shared_ptr to the UIDrawable derived object, then wrap whatever get/set color code (for a property or a SFML member object method) to pass get and set to the linked color object. Let's try and close this up: * PyLinkedColor properties: r, g, b, a, color * PyLinkedColor set(c) method * PyColor - just fix it entirely * Color names - I think I want to close the issue without solving this, and open a new one. It's such a fun feature, but it's not on the critical path.
john added spent time 2024-03-29 01:41:03 +00:00
2 hours
john added spent time 2024-03-29 03:51:02 +00:00
1 hour
john commented 2024-03-29 03:52:11 +00:00
Author
Owner
Copy Link
  • PyLinkedColor properties: r, g, b, a, color
  • PyLinkedColor set(c) method
  • PyColor - just fix it entirely

3991ac13d6

All three of these have had significant motion, but I need to chase down a segfault before this stuff can get cleaned up for merge

> * PyLinkedColor properties: r, g, b, a, color > * PyLinkedColor set(c) method > * PyColor - just fix it entirely 3991ac13d6471e491cbccf2ddb8d36bad528b2f7 All three of these have had significant motion, but I need to chase down a segfault before this stuff can get cleaned up for merge
john commented 2024-03-31 01:25:14 +00:00
Author
Owner
Copy Link

OK, we made it!

Ultimately, I have decided to abandon PyLinkedColor for now. After reflecting on the use cases, and looking ahead to doing the same thing to the Vector class, I did not have much interest in actually including the feature. I learned a bit more about lambdas and std::function, but sincerely it was an ADHD brain side-quest. Probably tripled the necessary time required to complete this feature.

Note: The segfaults were caused by not calling Py_INCREF on a new reference; the result is that print(c.fill_color) would segfault while color = c.fill_color would not. I think _ might have been accessing the null pointer.

Some cleanup actions are definitely being deferred, but I want to take the win now. It's my project, I can do that

OK, we made it! Ultimately, I have decided to abandon PyLinkedColor for now. After reflecting on the use cases, and looking ahead to doing the same thing to the Vector class, I did not have much interest in actually including the feature. I learned a bit more about lambdas and std::function, but sincerely it was an ADHD brain side-quest. Probably tripled the necessary time required to complete this feature. Note: The segfaults were caused by not calling `Py_INCREF` on a new reference; the result is that `print(c.fill_color)` would segfault while `color = c.fill_color` would not. I think `_` might have been accessing the null pointer. Some cleanup actions are definitely being deferred, but I want to take the win now. It's my project, I can do that
john added spent time 2024-03-31 01:25:27 +00:00
55 minutes
john closed this issue 2024-03-31 01:27:00 +00:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
master
grid_entity_integration
reprs_and_member_names
break_up_ui_h
standardize_font_handling
standardize_vector_handling
standardize_color_handling
standardize_texture_handling
raii_pyobjects
rebase-for-7DRL_Feb-2024
engjam_uicollection
engjam_ui_overhaul
Labels
Clear labels   
Alpha Release Requirement

This issue is a blocker to 0.1 Alpha release of McRogueFace.

  
Bugfix

   
Demo Target

Functionality to demonstrate; Depends on new features, but this issue isn't for writing them.

  
Documentation

   
Major Feature

Significant time and effort required.

  
Minor Feature

Some effort required to create or overhaul functionality.

  
Refactoring & Cleanup

no new functionality, just improving the codebase.

  
Tiny Feature

quick and easy - a few lines or with little interconnection around the codebase

No Label
Alpha Release Requirement
Bugfix
Demo Target
Documentation
Major Feature
Minor Feature
Refactoring & Cleanup
Tiny Feature
Milestone
Clear milestone
No items
No Milestone
All Datatypes Behaving
Projects
Clear projects
No project
The Datatype Overhaul
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Total Time Spent: 10 hours 16 minutes
john
10 hours 16 minutes
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: john/McRogueFace#11
No description provided.
Delete Branch "%!s(<nil>)"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?