Famous / Famous/engine

Refactor Mesh#onUpdate/ Color component

Open
#435 4 comments 0 reactions 0 assignees View on GitHub
controversial
Dominant language
JavaScript
Stars
1.7k
Forks
249
PR merge metrics
No merged PRs in 30d

Description

The `Color` component in `utilities/Color.js` has an opacity property that is being managed using a `Transitionable` (just like the `RGB` values).
- It doesn't make sense for a color component to have an opacity if the Node itself has an opacity (and possibly an `Opacity` component).
- It is also confusing since the opacity is _actually_ the alpha (which makes a lot more sense, after all that's what the A in `RGBA` is for).
- On the other hand, it actually is _not_ the alpha value, since the whole point of the alpha would be to enable alpha compositing, which is not the case, nor should it (that's what the node's opacity is for).

Aside from that, the `Mesh` itself - more or less - has a dependency on the Color component (or at least requires it to expose an `isActive` method), therefore the two concepts are not really decoupled.

I propose the following changes:
1. Remove Color component
2. Store color simply as a Mesh property
3. Don't `requestUpdate` **in** Mesh when a component is **active** (this violates the current abstraction - the Color component should do that) - see `Mesh#onUpdate`

``` js
Mesh.prototype.onUpdate = function onUpdate() {
// ...

if (node) {

// ...

// If any invalidations exist, push them into the queue
if (this.value.color && this.value.color.isActive()) {
// ...
this._node.requestUpdateOnNextTick(this._id);
}
if (this.value.glossiness && this.value.glossiness[0] && this.value.glossiness[0].isActive()) {
// ...
this._node.requestUpdateOnNextTick(this._id);
}
else {
// ...
}

// If any invalidations exist, push them into the queue
// ...
}
};
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.