HasDecorator with PositionComponent

[st-pasha]

What could be improved

Currently, the HasDecorator mixin works by wrapping the component's renderTree() with the decorator's apply() method. This works fine for position-independent decorators (such as those that manipulate the paint), but is problematic for decorators that need to know the component's coordinates.

Specifically, the PositionComponent in its renderTree method applies the coordinate transformation, which affects the component's render() and its children. However, this coordinate transform does not affect the decorator, which is applied outside. This is problematic for such decorators as Rotate3DDecorator or Shadow3DDecorator when the underlying component moves, since it requires some external logic to keep updating the decorators so that they are in sync with the underlying component.

How to improve

I'm currently considering the following approaches:

• Option 1: Add decorator property directly to the PositionComponent, so that its renderTree can apply the decorator after the coordinate transform.

  Pros:

  • Cleaner code;
  • No need to explicitly add HasDecorator mixin to position components;

  Cons:

  • Extra property decorator inside the PositionComponent class. Though if unused, it will be null, and won't take up much resources.

• Option 2: Inside the HasDecorator mixin check if the current component is a PositionComponent, and if it is, then perform renderTree which does not invoke super.renderTree but instead applies coordinate transform and renders children manually.

  Pros:

  • There is no decorator property unless explicitly requested by the user;

  Cons:

  • Duplicates code from renderTree methods of PositionComponent and Component;
  • More bug-prone in case the renderTree code is updated in those classes;

I'm leaning towards Option 1 here, but was wondering what do you guys think or if there's perhaps an Option 3 that I'm missing ?

[spydon]

Hmm, I'm undecided, seems like 1 is more convenient, but also it breaks the separation in a way that might not be very nice.

[spydon]

We discussed this on the meeting now and generally we wouldn't add new fields on PositionComponent, but this one is probably important enough to make an exception. So go ahead with option 1 if you want. Hopefully we can get this in before the release right? :)

[st-pasha]

I've been thinking about this, and here's what comes to mind:

1. Since PositionComponent is the ancestor for almost every other component in the game, we definitely want to make sure that it is as lean and efficient as possible. Which means we ought to be really cautious about adding new fields there.
2. Currently, PositionComponent is not as lean as possible: all its properties are ValueNotifiers, which means they contain lists of callbacks inside. Those lists, even if empty, increase the weight of the class.

• One way around this is to convert NotifyingVectors into regular vectors, but then have a special mixin that would override regular getters with those that produce notifying vectors. This way if the user wants to be notified about changes in a particular component, they'd have to request this functionality explicitly. Or we could just leave a single notifier on the overall transform2D matrix and do not expose separate notifiers for individual fields. This all would need further discussion, obviously.

3. Currently, the main function of the PositionComponent is to apply a transform2d matrix to the canvas before rendering the component and its children. This operation -- modifying the canvas around certain drawing operations -- is, by definition, a Decoration, even though we do not model it as such.

   But we CAN start modeling it like this. This would be option 3, in fact: model the current PositionComponent's operation as a decorator, and then tack on any additional decorators onto that through some chaining logic that we will add. This wouldn't be much different than option 1, but it could enable some interesting possibilities like replacing the canvas-transforming decorator with some other user logic, like introduction of skew or 3d transforms.

[spydon]

Option 3 sounds like quite a big breaking change, or would it be possible to do it in a non-breaking way? And would we be able to get this done before the release (which is on friday at the latest)?