Libgdx: Support Immutable Color

Created on 18 Sep 2015 · 20Comments · Source: libgdx/libgdx

We are running into an issue where a static Color stored in the Colors map is being changed - so that the shared Color used across the game changes also. (i.e "BLACK" was changed to no longer be black)

While we've fixed the cause of this, it isn't possible currently to make a Color object that enforces this. The r g b values are public, which means there is no easy way to subclass Color to make an "UnchangeableColor" class. There are hundreds of direct references to these public variables in the libGDX codebase.

Here's what i'd propose:

Replace the public float r, g, b, a with getters and setters.
Create an unchangeable Color class that is only set in the constructor and use it for colors like "BLACK" and "RED" which should never be accidentally changed, and optionally for any unchangable colors that a game wants to add to the Colors map.

Thoughts?

Source

ericnondahl

Most helpful comment

I apologize for bringing this up again, but I've had a similar design problem in my game engine and here's how I solved it:

public interface ColorInterface {
  public Color mutable(); // returns mutable verstion, or this if already mutable
  public ImmutableColor immutable(); // returns an immutable version, or this if already immutable
  public boolean isMutable();
  // getters and setters which accept floats, ints, packed ints, & ColorInterface
}
public class Color implements ColorInterface {
   public float r, g, b, a;
  // the normal color class which is still used everywhere
}
public class ImmutableColor implements ColorInterface {
  // bare implementation of ColorInterface which ignores SETs. Maybe throws an exception?
}
public class Colors {
  public static final ImmutableColor WHITE = new ImmutableColor(1f, 1f, 1f);
  // ... more colors
}

I still was able to use the Color class everywhere - and also added methods to accept a ColorInterface when it made sense. In some places I had ColorInterface (like the background color of the window) because it wasn't a place that was going to have heavy modifications to the color.

Not sure if this is of any help, but there it is!

ClickerMonkey on 19 Oct 2016

😄2 👍2

All 20 comments

Replace the public float r, g, b, a with getters and setters.

Will break a lot of code.

nooone on 18 Sep 2015

Please use the search. This has been discussed before.

xoppa on 18 Sep 2015

Also, libgdx source is plenty of static fields that should be ideally immutable: colors, Vector2.ZERO and Vector3.X are just some of them.
Personally I don’t think the change you're proposing is worthwhile, since it would break a lot of user code.

davebaol on 18 Sep 2015

Avoiding the design issue by saying "please use the search" is counter-productive. It has been discussed, but obviously not resolved from a design perspective.

It's very difficult to search a large code base for subtle changes to public variables that may be introduced by someone who doesn't know what they are doing with libGDX code. Better to design the class so that it keeps the private variables private to support subclassing.

In regards to breaking user code, it's unfortunate that it was designed the way it was, but sometimes we need to make API breaking changes to make the design less brittle.

ericnondahl on 18 Sep 2015

👍1

There was (and still is) a reason for designing it that way.

Color and similar classes like Vector2 are very basic and their properties are accessed and changed very often. Doing this via a getter and setter every time can potentially slow down the game, because of the method invocation overhead.

nooone on 18 Sep 2015

It seems likely that RoboVM would optimize that at the bytecode level for the iOS client. On android, similar optimization can happen via ProGuard, which is not difficult to add for the end user developer (and beneficial for other reasons.) I'm not sure the performance impact is a deal-breaker here, but it would have to be investigated before making a call.

ericnondahl on 18 Sep 2015

Seems like basic getters/setters are inlined by the JIT on Android as of Gingerbread (2.3). ART I would imagine might even do this AOT.

http://stackoverflow.com/questions/4912695/what-optimizations-can-i-expect-from-dalvik-and-the-android-toolchain/4930538#4930538

cjuillard on 18 Sep 2015

👍2

Don't forget about GWT.

You might be right and nowadays it might not be a serious problem anymore. But before someone actually benchmarks this, it is dangerous to just change it and assume it won't have an impact.

The fact that it would break every single existing libgdx application is a more serious problem though. Might be change for libgdx 2.0 maybe?

nooone on 18 Sep 2015

Let's not repeat the same discussion again, unless there's actually something new to add.

Btw, inlining would only be possible if there is only one implementation.

xoppa on 18 Sep 2015

Aside from the enormous breakages, and that colorInstance.setA(colorInstance.getA() + delta) is horrible to work with, this would also mean that we should do the same for all objects that we have 'constants' for, e.g. Vector as @davebaol pointed out. (To keep consistency across the API)

Imo the breakages and the resulting inconvenient API is not a good trade off for not getting bit by mutating mutable objects. Which is a fairly rare case, and one that happens only once.

This has been discussed before, as you can find in the github issues history with just a quick search, and the result was that these weren't to be changed.

Tom-Ski on 18 Sep 2015

Can someone link this old discussion? You may have too much faith in the issues search ;) The closest thing I see is this discussion on setColor in Actor, which is not what we are talking about here. https://github.com/libgdx/libgdx/issues/3380

ericnondahl on 18 Sep 2015

520, #2558

Tom-Ski on 18 Sep 2015

This goes way back (e.g. this) and every now and then it pops up again. I'm sure that if you search a bit more that you will find more. As you were asked to do before you opened this issue: "check if the issue has already been reported. Make sure to search all issues, not only the open issues."

xoppa on 18 Sep 2015

Ok I think we can close this and take those breaking changes into account for libgdx 2.0, if there ever will be.

davebaol on 19 Sep 2015

Please don't comment on this issue anymore. We've responded to your request, explained why it is how it is and why it is unlikely to be changed without a major version (if even then).

If you can't wait or have special needs from the library, please feel free to fork and maintain a copy having your desired modifications.

MobiDevelop on 19 Sep 2015

Not quite done with this yet. Thank you to the people who made substantive replies.

Just a bit more context as the pros and cons of this are weighed. I'd like to help other developers avoid the very tricky task of tracking down an unintended change to the static colors. We just found an issue that stemmed ultimately from this Color design choice, and it took a while (as in months) for testing to track this down. The changed color was in a completely different part of the game than where it became visible.

If we need to make an API change, that means compile errors, and while annoying, those are much easier to deal with than the unexpected changes in a static color or Vector2 that ideally would never change.

Performance and usability are legitimate concerns here, and it would be fine to kill off the idea if it could not be made to perform well.

Thanks for considering for an eventual change here. Sorry to bring up an old debate without realizing it!

ericnondahl on 19 Sep 2015

@ericnondahl as a side note: you can use Java's Color + converting util methods (toGdxColor(javaColor), toJavaColor(gdxColor)), preferrably using caching & pooling, to facilitate immutability, instrumentability & safety. It _will_ make your code slightly longer (since every call requiring a GDX Color will have to have the converter called), but for non-time-critical parts of the code it simply _won't_ matter, and for time-critical ones you'd want to stick with GDX's Color anyway for the sake of old Dalvik versions.

FyiurAmron on 25 Sep 2015

@FyiurAmron thanks, good idea. Currently trying a similar work-around for the issue by creating new Color objects by default instead of using the static ones (except for performance intensive places.)

ericnondahl on 29 Sep 2015

I apologize for bringing this up again, but I've had a similar design problem in my game engine and here's how I solved it:

public interface ColorInterface {
  public Color mutable(); // returns mutable verstion, or this if already mutable
  public ImmutableColor immutable(); // returns an immutable version, or this if already immutable
  public boolean isMutable();
  // getters and setters which accept floats, ints, packed ints, & ColorInterface
}
public class Color implements ColorInterface {
   public float r, g, b, a;
  // the normal color class which is still used everywhere
}
public class ImmutableColor implements ColorInterface {
  // bare implementation of ColorInterface which ignores SETs. Maybe throws an exception?
}
public class Colors {
  public static final ImmutableColor WHITE = new ImmutableColor(1f, 1f, 1f);
  // ... more colors
}

Not sure if this is of any help, but there it is!

ClickerMonkey on 19 Oct 2016

😄2 👍2

What I like in the above from ClickerMonkey is that you can actually find out in your code base where you are making the error of changing the color, by throwing an error for the ImmutableColor. Currently in libgdx there's no such way and I've been bitten by this as well.

Perhaps some debug/alternative version of color with setImmutable() and snapshoting color state could work, by adding debug checkpoints?