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

Add naked unit bug workaround #1406

Merged
merged 21 commits into from
Mar 3, 2021
Merged

Add naked unit bug workaround #1406

merged 21 commits into from
Mar 3, 2021

Conversation

Kexanone
Copy link
Contributor

When merged this pull request will:

Video for ZEN
cut.mp4

@jonpas
Copy link
Member

jonpas commented Jan 11, 2021

This contains commits from Malaria-infected civilians. Intentional?

Seems more fitting to put this into network component. @PabstMirror @commy2 ?

Fix acemod/ACEX#180

Possibly, I still don't think it will fix it 100% under some special conditions (indicating other problems though).

@jonpas jonpas added the Feature label Jan 11, 2021
@jonpas jonpas added this to the 3.16 milestone Jan 11, 2021
@Kexanone
Copy link
Contributor Author

Kexanone commented Jan 11, 2021

This contains commits from Malaria-infected civilians. Intentional?

Yeah, I forgot to mention that this should be merged after #1344, since it uses characters addon introduced in that commit.

Possibly, I still don't think it will fix it 100% under some special conditions (indicating other problems though).

It has at least all the parts we suggested and fixes the most obvious manifestations of the issue.

@Kexanone
Copy link
Contributor Author

Moved it to network

@commy2
Copy link
Contributor

commy2 commented Jan 12, 2021

Okay, but why do I have to trash my performance every time anyone connects to the server just because someone doesn't know how to use setUnitLoadout correctly?

@jonpas
Copy link
Member

jonpas commented Jan 12, 2021

just because someone doesn't know how to use setUnitLoadout correctly?

This doesn't have anything to do with the use of setUnitLoadout. This is an Arma bug with transferring units to different machines, ref. acemod/ACEX#163 and acemod/ACEX#180.

@jonpas
Copy link
Member

jonpas commented Jan 12, 2021

Could also add an activation gate, similar to player EH's - don't run if nothing is using it. If it can't be made fast enough in other ways.

@commy2
Copy link
Contributor

commy2 commented Jan 12, 2021

This is an Arma bug with transferring units to different machines

Why not let the one that transfers the units check this then? Maybe a function that does the transfering and does sanity checks for loadouts and whatever afterwards instead of this.

@jonpas
Copy link
Member

jonpas commented Jan 12, 2021

CBA shouldn't do any kind of transferring, that's too implementation specific. That also makes function implementation more convoluted. Ideally, we catch all bug states and fix them automagically.

@Kexanone
Copy link
Contributor Author

CBA shouldn't do any kind of transferring, that's too implementation specific. That also makes function implementation more convoluted. Ideally, we catch all bug states and fix them automagically.

I second that. In addition the bug also happens when Zeus disconnects, since the units get transferred to the server automatically. This can only be captured with a local EH.

@commy2
Copy link
Contributor

commy2 commented Jan 15, 2021

Still should be opt-in.

@jonpas
Copy link
Member

jonpas commented Jan 15, 2021

Still should be opt-in.

Agreed, ACEX Headless also requires specific enabling it, because not everyone is experiencing the bug in large amounts (most likely to do with amount of players).

commy2
commy2 previously requested changes Jan 16, 2021
Copy link
Contributor

@commy2 commy2 left a comment

Choose a reason for hiding this comment

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

make it opt in

@jonpas
Copy link
Member

jonpas commented Jan 23, 2021

Ok, I vote CBA Settings, it's the best way to go.

This fix ... won't help in 99% of played missions at all.

That's not true, it will help every Zeus mission and every session where a headless client with some kind of transferring is used. Those form a large percentage of players (Zeus use specifically, remember, Zeus units spawn on client, if client leaves or commands transfer, it is the same type of transfer as to a headless client).

Because people are dumb and check everything even if they don't understand what it means.

I don't know how this is relevant, we can't fix dumb people. You can only ever avoid dumb people by preventing them from using your product.

@Kexanone
Copy link
Contributor Author

Kexanone commented Jan 23, 2021

If you are worried about stupid users trashing your server, just force disable it. As requested, 37449af prevents now the validation to have any effect on your machine when you disable the option.

Edit: It's already disabled by default, but we could also set _isGlobal of CBA_fnc_addSetting to 1.

@jonpas
Copy link
Member

jonpas commented Jan 23, 2021

It is inherently a global setting, so that should be 1.

@Kexanone
Copy link
Contributor Author

It is inherently a global setting, so that should be 1.

Done

@jonpas
Copy link
Member

jonpas commented Jan 23, 2021

I forgot about this though

Edit: Actually, with the workaround as a CBA setting, you could already do this by only enabling it on the server.

But that isn't very clear at all, so if we add this it should probably be a drop-down setting with 3 options as described above.

@Kexanone
Copy link
Contributor Author

I forgot about this though

Edit: Actually, with the workaround as a CBA setting, you could already do this by only enabling it on the server.

But that isn't very clear at all, so if we add this it should probably be a drop-down setting with 3 options as described above.

As I mentioned in the same comment, it doesn't make much sense to have it one-directional. I just pointed out that you could have different setting on different machines before, not that it makes sense to do so.

@Kexanone
Copy link
Contributor Author

Oh wait, yeah. I almost forgot. The additional option to make it only applicable to playable units I suppose. Is it really needed?

@jonpas
Copy link
Member

jonpas commented Jan 23, 2021

That's the one I meant. I think it would be nice, that way you don't get any overhead on AI units? I didn't look at it enough to know if it would actually be any better.

@Kexanone
Copy link
Contributor Author

That's the one I meant. I think it would be nice, that way you don't get any overhead on AI units? I didn't look at it enough to know if it would actually be any better.

Alright, I added it as a third option.

Copy link
Member

@jonpas jonpas left a comment

Choose a reason for hiding this comment

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

Minor settings casing.

addons/network/initSettings.sqf Outdated Show resolved Hide resolved
addons/network/XEH_preInit.sqf Outdated Show resolved Hide resolved
addons/network/XEH_preInit.sqf Outdated Show resolved Hide resolved
addons/network/XEH_preInit.sqf Outdated Show resolved Hide resolved
Kexanone and others added 5 commits February 24, 2021 23:07
Co-authored-by: jonpas <jonpas33@gmail.com>
Co-authored-by: jonpas <jonpas33@gmail.com>
Co-authored-by: jonpas <jonpas33@gmail.com>
Co-authored-by: jonpas <jonpas33@gmail.com>
Co-authored-by: commy2 <commy-2@gmx.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Naked units transfer bug (#163) persists
5 participants