Skip to content

Color struct QOL tweak: Preview the built-in colors in-editor#28

Open
Demorome wants to merge 23 commits intoMoonsideGames:mainfrom
Demorome:help-upstream
Open

Color struct QOL tweak: Preview the built-in colors in-editor#28
Demorome wants to merge 23 commits intoMoonsideGames:mainfrom
Demorome:help-upstream

Conversation

@Demorome
Copy link
Contributor

@Demorome Demorome commented Nov 30, 2025

  • Most importantly, added RGBA hex value comments for each built-in color, which shows a preview of the color, at least in VS Code. Only works when the source file is opened, though; it doesn't show up when the summary comments appear when hovering over the static color name.

The rest is pretty minor:

  • Specified StructLayout to guarantee it'll be 32-bit packed (probably paranoid).
  • Added FromPacked() static constructor.
  • Added ToHexString() method.
  • Moved all built-in color declarations to their own #region at the bottom.
  • Use the byte-overloaded ctor for built-in colors since it has less operations (extremely minor, but while I'm here...).
  • Use namespace MoonWorks.Graphics; declaration instead of a scoped declaration to save horizontal viewing space.

@Demorome Demorome changed the title Help upstream Upgrade Color.cs Nov 30, 2025
@Demorome Demorome changed the title Upgrade Color.cs Color struct QOL tweak: Preview the built-in colors in-editor Nov 30, 2025
Copy link
Contributor

@thatcosmonaut thatcosmonaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is reasonable, just some nitpicks.

@thatcosmonaut
Copy link
Contributor

This is looking good, I'm going to try to split this into a content and a formatting commit.

@thatcosmonaut
Copy link
Contributor

thatcosmonaut commented Dec 20, 2025

Should we just remove PackedValue()? It's a pretty bad gotcha.

EDIT: I guess it's useful for ImGui interop...

@Demorome
Copy link
Contributor Author

Demorome commented Dec 20, 2025

I was going to rename it to UnsafePackedValue, at the very least.
I'm not sure if it's even useful for ImGui interop, since you can use ImGui.GetColorU32 on the vector-packed form of a Color to get it in the guaranteed format that ImGui will want. Though maybe currently using PackedValue for that just happens to work, I haven't tested it. Still, it's editor code, I don't need it to be that optimal...

@thatcosmonaut
Copy link
Contributor

Then let's remove it.

@Demorome
Copy link
Contributor Author

Huh, apparently using ImGui.GetColorU32 ensures that your color for ImGui is multiplied by style.Alpha: https://github.com/ocornut/imgui/blob/683f9160b96a2ae419d9a4896481b260e85a1b87/docs/FAQ.md?plain=1#L642
So in a way, using that function should be recommended anyways. Though there is an overload that lets you pass in a uint32.

@Demorome Demorome marked this pull request as draft December 20, 2025 03:04
@Demorome Demorome marked this pull request as ready for review December 20, 2025 04:05
@Demorome
Copy link
Contributor Author

Demorome commented Dec 20, 2025

I've removed PackedValue(), replacing it with BigEndianPackedValue() in case that functionality would prove useful. It doesn't seem nearly as much of a footgun, at least.

I've also added FromHexString, to complement ToHexString.

Otherwise, the other changes were pretty minor.

Since "byte-order" has confusing implications with endianness.
@Demorome
Copy link
Contributor Author

Demorome commented Dec 20, 2025

I was thinking of adding unit tests for these functions, using something like https://code.visualstudio.com/docs/csharp/testing, but I felt that adding a whole new project just for this PR would be a bit out of scope. Lemme know if you feel otherwise.

I would probably be using xUnit, since that seems to be the most popular. It would look like: https://xunit.net/docs/getting-started/v3/getting-started

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants