Skip to content
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

Explosives - Optimize creating explosive place actions #6413

Merged
merged 8 commits into from
Sep 22, 2018

Conversation

mharis001
Copy link
Member

@mharis001 mharis001 commented Jun 30, 2018

When merged this pull request will:

  • Optimize explosive place action (remove useless condition) and creating its children (optimized function and return is cached)

All of these were tested with count magazines = 28 and placeable explosives => 6, 2, 1, 1 (where each number represents the amount of a unique type of explosive).

  • The old function took ~0.31 ms
  • Useless condition on ACE_Place action took ~0.1 ms (time it takes hasExplosives)
  • New function takes ~0.20 ms (without any caching)
  • New function first time (with caching) takes ~0.23 ms (cachedCall adds ~0.03 ms)
  • New function (when return is cached) takes ~0.009 ms

Overall: Instead of ~0.41 ms every second, the new method is ~0.23 ms after a change in the player's loadout and then ~0.009 ms (accessing the cached value) every second after that.

For an inventory with no placeable explosives and count magazines = 17: it is ~0.11 ms vs ~0.10 ms (first time after loadout change) and ~0.009 ms every time after that.

@mharis001
Copy link
Member Author

The hasExplosives function is not used anywhere else after this change. The header says that it is a public function but it is not on the wiki. Safe to remove it?

Copy link
Contributor

@dedmen dedmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing to object. Beautiful work :3 Awoooo

@TheMagnetar
Copy link
Member

I would not remove a public function. Some people do look at the code and by removing it you may break somethinget like other mode or missions

@dedmen
Copy link
Contributor

dedmen commented Jun 30, 2018

hasExplosives still sounds useful. But atleast add that arrayIntersect fix to it

@jonpas
Copy link
Member

jonpas commented Jun 30, 2018

@TheMagnetar are you using it? If it's not marked "Public: Yes" it's not meant for public use. Of course if someone uses it still, it's up to them to pay attention and update it or request it to be made public API properly.

@PabstMirror PabstMirror added the kind/optimization Release Notes: **IMPROVED:** label Jul 1, 2018
@PabstMirror PabstMirror added this to the 3.13.0 milestone Jul 1, 2018
@alganthe
Copy link
Contributor

alganthe commented Jul 1, 2018

^ hasExplosive is indeed marked public jonpas.

imho if public funcs are removed a core dev should make an announcement on the forums and website to have the maximum visibility.

As to the usefulness of said func, i'd say it has it's place.

edit: it's used here @mharis001

@mharis001
Copy link
Member Author

That use is taken out in this PR, if the player has no explosives, there wont be any children actions and the action wont show anyway. Will update hasExplosives to be more efficient since it not being removed.

@jonpas
Copy link
Member

jonpas commented Jul 1, 2018

Oh maybe I should've read the other comments properly... Yeah, as a Public function we have to keep backwards compatibility. If it's essentially deprecated, then mark it as so with the ACE_DEPRECATED macro (+2 major versions from the one it was added in). No need to improve, only remove when the time comes! :D

@mharis001
Copy link
Member Author

The function isn't getting a better alternative, I just thought about removing it since it isn't used anywhere is ace. Still deprecate?


_result
_magazines arrayIntersect _magazines findIf {getNumber (_cfgMagazines >> _x >> QGVAR(Placeable)) == 1} > -1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(_magazines arrayIntersect _magazines) findIf {getNumber (_cfgMagazines >> _x >> QGVAR(Placeable)) == 1} > -1

@commy2
Copy link
Contributor

commy2 commented Sep 19, 2018

¯\_(ツ)_/¯

Copy link
Contributor

@dedmen dedmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙄 Awoooo

@PabstMirror PabstMirror merged commit 8648cca into acemod:master Sep 22, 2018
@mharis001 mharis001 deleted the optimize-explosive-actions branch September 22, 2018 05:34
@PabstMirror PabstMirror modified the milestones: 3.13.0, 3.12.4 Nov 9, 2018
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
* Optimize explosive actions

* Optimize hasExplosives function

* Readability parentheses

* bump

* testing...

* last try

* for science

* fix for sqf_validator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/optimization Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants