CBA_fnc_getVolume not calculating volume correctly

[Wakbub]

When merged this pull request will:

• Fix CBA_fnc_getVolume not calculating volume correctly
• Add an optional parameter to use greater or less precision in the calculation

The commands boundingBox and boundingBoxReal return an array with the extreme points of a model in the format [[x1, y1, z1], [x2, y2, z2]] (left–back–bottom, right–front–top). The current implementation of CBA_fnc_getVolume returns the product x2 * y2 * z2. That is the formula for the volume of a cuboid given x1, y1, and z1 are all 0. However, for boundingBox and boundingBoxReal they are not 0 but negative.

The proposed implementation calculates vectorDiff between [x2, y2, z2] and [x1, y1, z1] and returns the product of those elements. Similar calculations, for size instead of volume, can be seen in BIS_fnc_boundingBoxMarker (although that function takes for each element vectorDotProduct between the difference and its corresponding units vector—overcomplicated and slower) and BIS_fnc_objectHeight where the height is calculated as z2-z1. It is worth noting however that in the vast majority of BIS functions abs is used on the difference between the elements, which appears completely unnecessary as of all to judge the difference cannot be negative.

The proposed implementation uses boundingBoxReal by default for greater precision, while the current implementation uses boundingBox. This is motivated and does not break backwards compatability, as the current implementation does not return the volume in the first place.

The docstring does not explain the difference between boundingBox and boundingBoxReal other than in the example. This is intentional, as the user is assumed to have prior knowledge of those engine commands. Feel free to include it in the function description or parameter description if it is considered suitable.

As the proposed implementation is written from scratch the previous author has been omitted. If this is uncalled for, feel free to include the previous author for historical reasons.

Example output from the current and proposed functions on a Strider follows below.

Current implementation
[_strider] call CBA_fnc_getVolume
22.5603

Proposed implementation with less precision (using boundingBox)
[_strider, false] call CBA_fnc_getVolume
180.483

Proposed implementation with greater precision (using boundingBoxReal)
[_strider, true] call CBA_fnc_getVolume
104.446
[_strider] call CBA_fnc_getVolume
104.446

[commy2]

The math is right and nice usage of vector commands.

But:

Result:
0.0116 ms

Cycles:
10000/10000

Code:
[cheetah, false] call CBA_fnc_getVolume

Result:
0.0113 ms

Cycles:
10000/10000

Code:
[cheetah, true] call CBA_fnc_getVolume

These are exactly the same, and if you carefully read the code, it is clear why:

private _boundingBox = [boundingBox _object, boundingBoxReal _object] select _real;

This ^ line is executed in the following order:

1. boundingBox returns
2. boundingBoxReal returns
3. Array made consisting of the return values of 1 and 2
4. Either element 1 or 2 are "selected" from the array and stored in the _boundingBox variable

What you expected to happen was that only either boundingBox or boundingBoxReal are executed. But this would've have to been written as:

private _boundingBox = call ([{boundingBox _object}, {boundingBoxReal _object}] select _real);

or more commonly used although ugly af:

private _boundingBox = if (_real) then {boundingBoxReal _object} else {boundingBox _object};

This is good news, because it means this function can be made even faster.

Personally I don't see the reason not to just always use boundingBoxReal. It is more accurate and still bigger than the actual vehicle (but not as much "too big" as boundingBox). The performance difference between those two commands is neglible. The overhead for handling both versions in one function is probably bigger than the advantage of boundingBox over boundingBoxReal considering SQF biggest time sink is input validation from commands.

[commy2]

broken optimization

[Wakbub]

I did compare select vs if–else before submitting but found no noticeable difference on a Strider. After several runs (not just one) they all (select, call, and if–else) land at 0.0104. With a Cheetah it varies too much to be able to say which code is faster. But as you seem to know how it works and what is/should be faster, we'll stick with your suggestion. (Note that what I wrote above were units of volume. It would make no sense to compare the current and proposed implementation in terms of speed when the current does not return the volume.)

select

Code:
[cheetah, false] call CBA_fnc_getVolume;
Result:
0.0627, 0.063, 0.0653

Code:
[cheetah, true] call CBA_fnc_getVolume;
Result:
0.0631, 0.0643, 0.0654

Cycles:
10000/10000, 10000/10000, 10000/10000

call

Code:
[cheetah, false] call CBA_fnc_getVolume;
Result:
0.0634, 0.065, 0.0654

Code:
[cheetah, true] call CBA_fnc_getVolume;
Result:
0.0622, 0.0647, 0.0651

Cycles:
10000/10000, 10000/10000, 10000/10000

if–else

Code:
[cheetah, false] call CBA_fnc_getVolume;
Result:
0.0624, 0.0644, 0.0654

Code:
[cheetah, true] call CBA_fnc_getVolume;
Result:
0.0638, 0.0649, 0.0652

Cycles:
10000/10000, 10000/10000, 10000/10000

The rationale for supporting both commands is simply that users might be interested in both. boundingBoxReal was introduced in Arma 3 version 0.50, but for some reason all BIS functions below introduced in Arma 3 version 1.00 use boundingBox when calculating bounding boxes.

BIS_fnc_camera
BIS_fnc_diagWiki
BIS_fnc_effectFiredFlares
BIS_fnc_effectFiredSmokeLauncher
BIS_fnc_effectFiredSmokeLauncher_boat
BIS_fnc_moveToRespawnPosition

Without studying the functions further, it's probably safe to assume they don't calculate volume. However, there might still be use cases where the volume of boundingBox is desirable. Though if you consider it better to skip it we'll do.

[commy2]

I see. I misunderstood you then. If both of these commands are so fast that executing both makes no noticable difference in speed, and only the returned value changes, then it is probably not worth it to worry about optimization.
Or to get an explanation of why it appears to make no difference: select is by far the faster control structure, and both commands, bb and bbr are almost instant.

[Wakbub]

Although evaluating
call ([{boundingBox _object}, {boundingBoxReal _object}] select _real)

isn't noticeably different (sometimes slower, sometimes faster) than evaluating
[boundingBox _object, boundingBoxReal _object] select _real

it is certainly true that just evaluating
boundingBoxReal _object;

is faster than evaluating both

boundingBox _object;
boundingBoxReal _object;

which for reasons of speed warrant two functions as you mentioned. The only problem is that the function you'd likely most want to use would then be CBA_fnc_getVolumeReal (if they should be named in line with the engine commands). And now when it is possible to name the mostly desired one just CBA_fnc_getVolume, it would be unfortunate not to do so. This is why the command choice was included as a parameter. But if you can think of a good name for a function that uses boundingBox I would approve of it. I'm pleased to make any changes as long as CBA_fnc_getVolume returns volume using boundingBoxReal one way or another.

[commy2]

I think it is fine how it is, even though I personally see no reason why anyone would ever even want to use bb instead of bbr.

[Wakbub]

Neither do I, but sometimes people have inconceivable desires. Maybe we should ask BI why.

If someone ever wants to use boundingBox when calculating volume they are welcome to request it.