-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check callout validity #380
Conversation
Removes basically all performance penalty from the validity check
private _sound = toLowerANSI _x; // playSound3D is case-insensitive | ||
|
||
// File extension must exist for playSound3D to work | ||
if (_sound == "" || !(".ogg" in _sound || ".wss" in _sound || ".wav" in _sound)) then { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering in
is rather slow on strings, RegEx might be faster (it's been faster before).
Alternatively, assuming there can't be anything after the extension for the sound to be considered legal, doing an end-of-string select
may be even faster than a full-string RegEx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overengineering will result in a slowdown.
String to benchmark with (just smashed my keyboard):
_sound = "a3479tghuaighreuyigvhbierlagheriap9tg745ra3heugjreivbugav7843fgrhbg864753g453hqb76453q9g54rgbh6q793g84gfcy43q.wav"
Benchmarks:
!(".ogg" in _sound || ".wss" in _sound || ".wav" in _sound);
~0.0013ms
(_sound regexFind ["(.*?)\....", 0]) isEqualTo []
~0.0030ms
End-of-string select (not even doing the compare)
_end = _sound select [((count _sound) - 4)]
~0.0013ms
An argument could be made for the RegEx, because even though it is slower, it will match any file extension. But I don't believe the engine even supports that many formats, mp3 maybe? But mp3 could just be added to the list of checks if it is supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try regexMatch
instead of regexFind
if you don't need the match itself, and if you check just the extension it can be done by the .+?\.(?:ogg|wss|wav)$/io
pattern (or .+\.(?:ogg|wss|wav)$/io
if the former is somehow slower, or even more weirdly .++\.(?:ogg|wss|wav)$/io
).
I admit I can't test it right now and select
's docs make me suspicious, but it should support end-of-string/-array selections through negative indexes since 'Arma 3 v2.14':
private _end = _sound select [-4]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I recall, all callout sounds in vanilla are in .ogg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want to go after that, the fastest here is to check if a .
is in the file path, though I don't think the speed is needed.
However, the question is if we maybe check if a file with the "supported" file endings exists so that we don't "filter" out still possible valid callouts and increase mod support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative method would just be to run fileExists
on it. That would guarantee that everything is valid, but it might be an order of magnitude slower.
I'd appreciate it if somebody could run the benchmarks too for the current method, the regexMatch
method and the fileExists
method. I think regexMatch
might be an improvement over the current one (even with a slight performance regression), as it's a much more compact statement and it's easier to add more filetypes (maybe mp3).
Superseded by #397 |
When merged this pull request will: Title
Describe what this pull request will do
Check that callouts are valid when caching them.
Negligible performance degradation to caching logic (
~0.0012ms
per check) but overall positive performance impact once cache is made.This should remove all log errors relating to invalid files, e.g:
Sound: Error: File: rhsafrf\addons\rhs_s_radio\radio\Male01\RU\Advance not found !!!
Each change in a separate line