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

Arsenal - Fix FUNC(removeVirtualItems) for JIP players #9650

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

johnb432
Copy link
Contributor

@johnb432 johnb432 commented Nov 15, 2023

When merged this pull request will:

The side effect is that all changes are stored in the JIP queue until the arsenal or its object is deleted. I'm not sure how to handle this any better.

IMPORTANT

  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
  • Development Guidelines are read, understood and applied.
  • Title of this PR uses our standard template Component - Add|Fix|Improve|Change|Make|Remove {changes}.

Copy link
Contributor

@LinkIsGrim LinkIsGrim left a comment

Choose a reason for hiding this comment

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

This should work (I can't test for a while), but I'm not a fan.

Did this work properly in 3.15.2?

addons/arsenal/functions/fnc_openBox.sqf Show resolved Hide resolved
@LinkIsGrim LinkIsGrim added the kind/bug-fix Release Notes: **FIXED:** label Nov 15, 2023
@johnb432
Copy link
Contributor Author

This should work (I can't test for a while), but I'm not a fan.

I'm not a huge fan because of the possible growing JIP queue, but this methods technically allows the most flexibility. What are your concerns?

Did this work properly in 3.15.2?

Given the setup wasn't that much different from what is now in 3.16.1, I think this has always been broken. But at the same time, I would be surprised if that were really the case, someone would have certainly complained about it sooner.

https://github.com/acemod/ACE3/pull/9040/files#diff-be69c81d7a52a673c5725994e6c9d11c944400be668ce2d33b9290e2e84ecbe2 #9040 did some minor changes to FUNC(initBox), but I don't think those are to blame for it being broken.

I'm going to check if it was working in 3.15.2 later today.

@LinkIsGrim
Copy link
Contributor

I'm not a huge fan because of the possible growing JIP queue, but this methods technically allows the most flexibility. What are your concerns?

JIP queue is the only concern. Ideally there wouldn't be enough changes made over the course of a synced arsenal's lifetime for it to be an issue, but it can have an impact when a player joins. Whether it's significant enough to worry about, I don't know. I'm not worried about desync, though.

@johnb432
Copy link
Contributor Author

Did this work properly in 3.15.2?

I can confirm that this did not work in 3.15.2, it's been broken for god knows how long.

[...] but it can have an impact when a player joins. Whether it's significant enough to worry about, I don't know. [...]

I'm not sure how to go about it in a better way unfortunately.

@LinkIsGrim
Copy link
Contributor

If it didn't work in 3.15.2 then this is good enough of a fix. Won't approve because I can't test, but code LGTM.

@johnb432
Copy link
Contributor Author

I'm not sure how to go about it in a better way unfortunately.

Better way was found...

addons/arsenal/functions/fnc_openBox.sqf Outdated Show resolved Hide resolved
!isNil {(_this select 0) getVariable QGVAR(virtualItems)}
}, {
[_this select 0, _this select 1, true] call FUNC(removeVirtualItems);
}, [_object, _items], 20] call CBA_fnc_waitUntilAndExecute; // 20s timeout in case of failure
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if you are saying it's lit or if it should be set on fire...

Copy link
Contributor

Choose a reason for hiding this comment

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

the-road-to-el-dorado-both

@johnb432
Copy link
Contributor Author

The arsenal now refreshes globally if FUNC(addVirtualItems) or FUNC(removeVirtualItems) was applied globally.

@johnb432 johnb432 changed the title Arsenal - Fix FUNC(removeVirtualItems) for JIP players Arsenal - Fix FUNC(removeVirtualItems) for JIP players Nov 17, 2023
@LinkIsGrim LinkIsGrim added this to the 3.16.2 milestone Nov 17, 2023
@LinkIsGrim LinkIsGrim merged commit 8f3129a into master Nov 17, 2023
5 checks passed
@jonpas jonpas deleted the remove-virtual-items branch November 17, 2023 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug-fix Release Notes: **FIXED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants