ahve -> have

Ahh thanks, fixed. I also noticed in the setter above I was accessing the gettr instead of the instance var. Changed that too.

Without this patch, this test can pass also, right?

The container gets rendered in the wrong location without this patch

Err, sorry - without this change, there'd be no OpacityLayer, the PictureLayer would be the firstChild of rootLayer.

It passes with the latest master branch.

Ahh you are right. This would need to test that the add to scene does the right thing. But the golden test does that without getting into such implementation details. I'll remove this test.

It isn't bad to remain this ^^

I agree, it's not bad to keep this test, even if it passes without this change and has coverage in a golden file test. I think it's never bad to have another test. :)

Left it in :)

I agree, it's not bad to keep this test, even if it passes without this change and has coverage in a golden file test. I think it's never bad to have another test. :)

The type of the engineLayer is specific to the implementation of the addToScene method. Rather than null it here where the reason for the nullification isn't immediately obvious, deal with the wrong type down in addToScene.

Basically in addToScene, only pass along the oldLayer if it is of the right type and it will get replaced naturally.

Because addToScene is on a hot path, and will require type checks every time that hot path gets hit, whereas we know what the type will be based ont he value of the opacity.

A single is is going to completely derail our performance?
Compared to the improved maintainability by not having multiple functions have to deal with an implementation detail of one of them?

addToScene has to do an as anyway which implicitly includes an is, but throws if it fails rather than just substituting null which is the logical reaction to the type being wrong. Note that with the is you likely don't even need the as because the runtime is smart enough to elide them.
You are already paying the price of type checking the parameter in addToScene, why complicate it?

So I figured I don't have a benchmark to back this up and I can just make this change. However, there's another complication here. What I really want is it to be an error to call addToScene with an incorrect engineLayer, because you cannot mutate the engineLayer during addToScene, and if we got called addToScene without nulling out the no-longer-needed engineLayer, we've made a mistake that wastes memory.

I'm happy to update the documentation on this if it's not already updated, but I think the asserts and mutations are correct this way, and we'd risk recurisvely calling addToScene if we start trying to mutate the engineLayer there.

How is "this object wants to decide on the fly which type of engine layer to use when it adds to the scene" compatible with "addToScene is not allowed to mutate the engineLayer and we will make that an error"?

Philosophically those two concepts are at odds with each other. You can't justify one without pointing out that the other is or has an invalid assumption.

Adding code to prevent a conflict between them is a workaround and points out that the design decisions had some cases they didn't really address.

You are basically nulling out the field to destroy the evidence that the engine layer is actually dynamic here.

And if there is a reason for it to be dynamic here, then what is the value of a globally enforced assertion that it isn't.

(None of this is blocking, btw, I'm just a little skeptical over the design constraints that led to this implementation...)

While we're here, should this be > 0? Some places that check alpha use an inequality and other places use equality tests (see RenderAnimatedOpacityMixin below).

I see that there are some asserts in here, but not sure if they protect every value test or not.

Fair, I'll fix this. Previously we couldn't use that because 255 was also special cased :)

Here we use inequality for alpha test, other places use equality testing. Perhaps all tests on alpha should use tests that are safe in the presence of out-of-range values like this?

Also, there are range asserts sprinkled around, but it is hard to tell from a PR diff if they are consistently protecting every line of code.

We use the following code from dart:ui to calculate alpha

  static int getAlphaFromOpacity(double opacity) {
    assert(opacity != null);
    return (opacity.clamp(0.0, 1.0) * 255).round();
  }

It's guarnateed to not go out of bounds.

Inter-method dependencies for correctness are weaker than just having this method deal with the wrong layer type that it knows happens because it is the method that causes it.

The asserts in here guard against this getting broken, and there is test coverage that would trigger them should it break. I did this becuase it's on a hot path.

What is the value of subclassing OffsetLayer if we push our own?

Eliminates duplicated code with regard to setting the transform, and both layers provide an offset property.

Users of this API don't really deal directly with scene builder - but in some of my explorations around trying to optimize layer usage, it can be helpful to know whether a layer has an offset or not.

No range assert here.

That should be addressed in a separate PR.