About scratch variables and API consistency

[javagl]

The concept of "scratch" variables and the pattern of
result = SomeClass.doSomething(input, scratchResult);
with the optional result parameter is important for performance-critical applications. It offers the functionality of
SomeClass.doSomething(input, result);
and
result = SomeClass.doSomething(input);
in a single function, optionally(!) leaving the aspect of memory management to the developer.
(Slightly more complex than each of the simpler ones, but the "best of both worlds")

But some of the API and corresponding documentation should be more strict and more consistent. Right now, whenever I call a function, I do not know ...

1. whether the result parameter is optional
2. whether the return value will be the result parameter

---

Point 1. can usually be looked up.

Regarding the consistency, one could notice:
Cartesian3.clone(cartesian, result)
Cartesian3.negate(cartesian, result)
For clone, the result is optional. For negate, the result is not optional.

Why?

I think that the answer is: The result is optional for functions whose sole purpose is to create instances, like clone or from..... If this is the case, it could/should be pointed out in the coding guide. And one should keep an eye out for places where this rule is not followed. I think that there are a few places where this is currently not the case.

One could make a case to always make it optional, so that one can write result = Cartesian3.negate(input);. But I see the risk of "accidentally" creating instances, particularly by users of the API who may not be aware of the implications of this call.

One general thing that should be kept in mind:

• Changing a function to make the result non-optional would be a breaking change
• Changing a function to make it optional should be safe.

---

Point 2. is more subtle.

The Coding Guide at https://github.com/CesiumGS/cesium/tree/main/Documentation/Contributors/CodingGuide#result-parameters-and-scratch-variables says:

Because result parameters aren't always required or returned, don't strictly rely on the result parameter you passed in to be modified. For example:
Cartesian3.add(v0, v1, result);
Cartesian3.add(result, v2, result);
is better written as
result = Cartesian3.add(v0, v1, result);
result = Cartesian3.add(result, v2, result);

Now, this does raise one crucial question:
Under which conditions is the result parameter not returned?

Not being clear about this may lead to obscure patterns (and maybe bugs). One example (not to 'blame' anyone, but just something that I stubled over) is in the InstancingPipelineStage . It receives the nodeComputedTransform. And it uses it like
nodeComputedTransform = Matrix4.multiplyTransformation(...nodeComputedTransform);

Now there are two options:

1. when the return value is the given result parameter, then the assignment does not make sense
2. when the return value is not the given result parameter, then call does not make sense

In any case, something does not make sense 🙂

The main point is: It should be possible to rely on a certain behavior and consistency here.

[ggetz]

Thanks for thinking this through @javagl.

For background, #1161 describes why some functions always require the result parameter, namely performance. #7444 also describes from... function which are similar to clone.

I would also say point 2 can be looked up in the documentation as well. The function should be documenting that the return value is the resulting object or undefined under some cases.

Given this is so ingrained in the codebase, and retroactively changing the API could lead to further confusion, I'm leaning towards closing this issue. Let me know if I'm off base anywhere.

[javagl]

I'm still wondering about the justification for the recommendation from the Coding Guide. Why should it be
result = Cartesian3.add(v0, v1, result);
and not
Cartesian3.add(v0, v1, result);
?

This is related to that nodeComputedTransform usage. It might be that this was exactly caused by someone following the coding guide, literally, even though it does not make sense in this case (and may be confusing).

(The issue can remain closed when there is no actionable item - i.e. assuming that the coding guide will not be modified...)