Skip to content

feat(JOML): migrate Color#8

Merged
skaldarnar merged 3 commits into
MovingBlocks:masterfrom
pollend:feature/color
Aug 9, 2020
Merged

feat(JOML): migrate Color#8
skaldarnar merged 3 commits into
MovingBlocks:masterfrom
pollend:feature/color

Conversation

@pollend
Copy link
Copy Markdown
Member

@pollend pollend commented Aug 8, 2020

This is a rework of how colors work in nui. This is more consistent with JOML. I think this configuration should be easier to use. toVector3f, toVector3i and toVector4f are not used in any of the modules under omega or the engine. If its not used then It would probably be ok to just remove the methods entirely. I also update some of the documentation at the same time. There are some cases I'm probably not considering.

ref: MovingBlocks/Terasology#4118

@pollend pollend changed the title WIP: feat(JOML): migrate color feat(JOML): migrate color Aug 9, 2020
@pollend pollend marked this pull request as ready for review August 9, 2020 02:34
Copy link
Copy Markdown
Member

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

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

I was confused at first, but now the changes do make sense 👍

One addition (for a follow-up PR to keep the feature separate) would be to support color created and modification in other "formats" such as HSB. This would be helpful to do gradients easily in UI elements.

public class Color {
public class Color implements Colorc{

@Deprecated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Deprecating these sounds weird to me - isn't Color the class we want to use?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See comment below, confusion about Colorc was resolved. Just to ensure I get this right - in case I want to have a half-transparent green somewhere I'd need to do something like this, right:

Color transparentGreen = new Color(Color.green).setAlpha(0.5f);

Would it make sense to offer both a read-only value for the base colors and a static method that returns a new modifiable value? So for each base we would have a pair of

public static final Colorc WHITE = new Color(0xFFFFFFFF);

public static Color white() {
	return new Color(WHITE);
}

The example above could then become

Color transparentGreen = Color.green().setAlpha(0.5f);

@Deprecated
public static final Color MAGENTA = new Color(0xFF00FFFF);

public static final Colorc black = new Color(0x000000FF);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we want to expose only Colorc instead of Color? Should this rather be a helper class for working with JOML's color?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, now I got that Colorc is our own doing 🤓 (similar to what JOML does) 💡

@skaldarnar
Copy link
Copy Markdown
Member

Note: This PR addresses the current state of NUI (gestalt v5). PR #4 needs to be updated in case we merge this first.

@skaldarnar skaldarnar changed the title feat(JOML): migrate color feat(JOML): migrate Color Aug 9, 2020
@skaldarnar skaldarnar merged commit dd1f784 into MovingBlocks:master Aug 9, 2020
@pollend pollend deleted the feature/color branch August 9, 2020 15:42
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