This causes 40B of allocation each frame, though! This is because GetNearest does this:
public NNInfo GetNearest (Vector3 position) {
return GetNearest(position, NNConstraint.None);
}
And NNConstraint.None is a getter that creates a new Constraint.
To avoid it, I’ve just cached the constraint used in GetNearest. You should probably do that in the main release to avoid the same issue for all users!
It could be that None could be generally cached, but since it has a bunch of public fields, I don’t know if there’s a chance that the constraint might be edited after it’s constructed. That seems like a bad idea. Should the fields maybe all be readonly?
I completely agree with you, the problem is backwards compatibility.
The NNConstraint class has existed for a long time and a lot of code depends on it being mutable.
One common pattern is to get an instance from NNConstraint.None or NNConstraint.Default and modify that. This makes it really hard for me to introduce any change that makes it immutable or caches the None and Default properties.
I have been trying to figure out a good solution for some time now. Do you have any suggestions perhaps?
public interface INNConstraint {
int graphMask { get; }
}
public interface IMutableNNConstraint : INNConstraint {
new int graphMask { get; set; }
}
public class NNConstraint : IMutableNNConstraint {
public int graphMask { get; set; }
private static INNConstraint _immutable = new NNConstraint();
public static INNConstraint Default_Immutable {
get { return _immutable; }
}
public static INNConstraint Default_Mutable {
get {
return new NNConstraint();
}
}
}
So using that pattern, you could replace all NNConstraints with INNConstraint where possible, and use INNConstraintMutable otherwise. You could then have a INNConstraint.Default that’s a constant value, which will save on allocations. It’ll work for client code as long as nobody’s used variables of NNConstraint as ref parameters to methods, and if they have, rewriting won’t be hard.
The problem is that it might be a bit esoteric? I didn’t know you could re-use property names in interfaces like that before I tried it. It’s also possible to break everything by getting the Default_immutable, casting it to mutable, and changing it. You could work around that by having the implementations be two different classes, but that might break backwards compatability? I don’t know.
It’s honestly not a big problem, I can just open the profiler, figure out where allocations are happening, and fix that. It does mean that I have to re-fix it to update versions, which is a bit annoying, but not a killer.
Okay, I upgraded to 4.1.10, and looked through the code to figure out what to do.
What I ended up with was a pretty simple solution: be explicit about if a constraint is mutable or not. I turned NNConstraint into an interface, with two subclasses: NNConstraintMutable and NNConstraintImmutable. Both of these have a .None and .Default static property, where the Immutable one returns a cached version.
Turning all NNConstraints immutable was not viable, as pooled paths need to update their constraint. Turning them into structs seemed iffy - I think the performance impact of copying the constraint struct all over the place would be worse than the cure.
The solution I ended up with allows for the vast majority of the old code’s constraint-related allocations to be removed, without changing much code at all.
If you’re interested in implementing this upstream, I can send you the code.
Checking the code, the constraint is checked for null and produces the same result!
Changing that code to just use null works perfectly well and removes the allocation.
This one’s using Default assignment to reset. I added a Reset() to NNConstraint that assigns values in the None builder and overrode in PathNNConstraint to set constrainArea per the Default builder. Had Reset() check if non-null and simply call Reset(). Works fine.
Would these changes be reasonable to integrate? They seem to remove the only allocations I was seeing.
