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

WIP: Arma vanilla magwells #1030

Closed
wants to merge 4 commits into from
Closed

WIP: Arma vanilla magwells #1030

wants to merge 4 commits into from

Conversation

dedmen
Copy link
Contributor

@dedmen dedmen commented Nov 21, 2018

This is how it would look if we replace our magwells by the vanilla ones.
Need feedback. What do we do?

Leave the BI magwells be and just make our own duplicates?
Add inheritance? Problem with that is, mods that already add onto the CBA classes will break when we add inheritance. But right now it's soon enough that we can still do it.

Also the BI magwells are missing the reload tracers
30Rnd_556x45_Stanag_green, 30Rnd_556x45_Stanag_red and the mx variants.

@jonpas
Copy link
Member

jonpas commented Nov 21, 2018

Need feedback. What do we do?

Inherit CBA ones from BI ones, remove 1 or 3 versions later.

@jonpas
Copy link
Member

jonpas commented Nov 21, 2018

Drop the "This is" and just write // DEPRECATED! Don't use! Use vanilla one instead!. The shorter it is, more people read it.

@commy2
Copy link
Contributor

commy2 commented Nov 21, 2018

You should name "the vanilla one".

@jonpas
Copy link
Member

jonpas commented Nov 21, 2018

DEPRECATED! Use 'BI_wellName'!

@robalo
Copy link
Contributor

robalo commented Nov 21, 2018

I would like to see how the vanilla classes look like. Where are they ? Dev ? RC doesn't have any.
The mix of non-tagged with CBA tagged classes looks a bit messy at first sight.

@robalo
Copy link
Contributor

robalo commented Nov 21, 2018

My 0.02€

Unless vanilla implementation promises to get as developed as the current CBA one, deprecation should work the other way around. Deprecate vanilla classes and use a the complete and consistent CBA classes.

@Kllrt
Copy link
Contributor

Kllrt commented Nov 21, 2018

@robalo They look like this https://pastebin.com/bfa806BH

@Soldia1138
Copy link

What’s about e.g. the 100 Round MX magazines? The are commented out in the pasteboard but belong to the same config like the 30 round MX.
CBA has two different entries there.
The potential of conflicts is pretty high with a mixed approach.

Would be easier for us to stay on CBA-only magwells

@dedmen
Copy link
Contributor Author

dedmen commented Nov 22, 2018

Weapon and magazine mod makers will always want to use the Vanilla variant, because they also want to be compatible with users who are not using CBA.
Meaning if we decide to keep a duplicate, they will have to add their magazines to vanilla and CBA. And they will also have to consume the magazines from vanilla and CBA, causing duplicates everywhere.
We can't really deprecate vanilla classes, as we cannot just force every weapon mod maker to depend on CBA.

@robalo
Copy link
Contributor

robalo commented Nov 22, 2018

I'd rather have just a few duplicates - seeing how few the vanilla classes are (if those are real) - than confuse the hell out of everyone with a mixed approach.

@dedmen
Copy link
Contributor Author

dedmen commented Nov 22, 2018

if those are real

@Kllrt is a BI dev btw.

@robalo
Copy link
Contributor

robalo commented Nov 22, 2018

if those are real

@Kllrt is a BI dev btw.

I had no idea :D 🙇

@PabstMirror
Copy link
Contributor

If we are going to keep vanilla magwells I guess we might want something like

class arifle_MX_Base_F: Rifle_Base_F {
    magazineWell[]+= {"CBA_65x39_MX", "CBA_65x39_MX_XL"};

@dedmen
Copy link
Contributor Author

dedmen commented Dec 2, 2018

Btw why do we indent everything in the include files by one level?

@commy2
Copy link
Contributor

commy2 commented Dec 2, 2018

Waste of space.
The #include is already indented.

@robalo
Copy link
Contributor

robalo commented Dec 3, 2018

Noticed the magwells were added in latest update. IMO to keep the implementation clean we should go with inheriting from the base classes. Even if it will cause some updating classes RPT errors, for existing JAM-based configs, it won't break the game and they will be spotted and fixed easily.

What I don't like and expect will happen regardless of how we do it is modders throwing all their mags in the vanilla classes without care for the standard/large/drums etc distinction we have already. I still think that we should inverse the deprecation messages and say for example don't use AK_762x39, use CBA_762x39_AK and CBA_762x39_RPK instead.

@dedmen
Copy link
Contributor Author

dedmen commented Dec 4, 2018

we should go with inheriting from the base classes

Inheritance was broken. And I didn't hear of a fix so far.

Theoretically.... Could we do both? Have a inherited CBA class (once inheritance is fixed) and a seperate one too? Nah... I guess that will make things worse? Not sure.

So far I'm on the "deprecate vanilla ones" side too, even though I still don't like having everything twice...

@commy2 seperate PR to fix that someday?

@robalo
Copy link
Contributor

robalo commented Dec 4, 2018

Hmm, dunno about inheritance broken. Seems fine to me:
image

@PabstMirror
Copy link
Contributor

won't that lead to problems with mod load order?

@robalo robalo mentioned this pull request Dec 4, 2018
class GL_3GL_F : UGL_F {
magazineWell[] = {"CBA_40mm_3GL", "CBA_40mm_M203", "CBA_40mm_EGLM"};
magazineWell[] += {"CBA_40mm_3GL", "CBA_40mm_M203", "CBA_40mm_EGLM"};
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no 40mm magwell in vanilla.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+= won't break if the array exists right? So it doesn't really matter

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but that's the thing, doesn't exist, same thing on the UGL_F parent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually meant the opposite as what I said. So does += here break in it's current state? or is it just a minor issue?

Copy link
Contributor

@robalo robalo Dec 6, 2018

Choose a reason for hiding this comment

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

Last I checked this kind of array addition combined with inheritance was really finicky.
The above bits seem fine though, did a quick check and magazineWell is populated.

@robalo
Copy link
Contributor

robalo commented Dec 4, 2018

How about this stupid idea: we go with all new untagged classes, vanilla style, extending current vanilla implementation, deprecating everything CBA_ in magwells.

@Drofseh
Copy link
Contributor

Drofseh commented Dec 4, 2018

It might work, but it'll be kind of a pain.

@jonpas
Copy link
Member

jonpas commented Dec 4, 2018

Ideal would be if we could give extra classes to BI to add to vanilla... @Kllrt!

@dedmen
Copy link
Contributor Author

dedmen commented Dec 5, 2018

Hmm, dunno about inheritance broken. Seems fine to me:

Yes. But Arma doesn't see it that way. The magazine code doesn't look through parent entries.

Ideal would be if we could give extra classes to BI to add to vanilla..

And take a month or two everytime someone notices that something is missing?

Ideal would be to put everything of CBA into vanilla... Sooo...

@jonpas
Copy link
Member

jonpas commented Dec 5, 2018

Well that's what you get, it's in vanilla, that's a big enough pro for a small wait of 2-3 months.

@robalo
Copy link
Contributor

robalo commented Dec 5, 2018

So which way from here:

  1. Stick with what we got done, vanilla and CBA magwells can live happily side by side attached to weapons as modders see fit depending on whether they need/want to support CBA JAM.
  2. Figure out new class naming convention that looks consistent with vanilla naming and still makes sense and propose BIS to include them or at least be aware of them.

For 1. I still feel that we should not deprecate existing CBA magwells in favor of vanilla, Keeping them may cause some duplication to pop up but don't see what harm that may cause really.

@jonpas
Copy link
Member

jonpas commented Dec 6, 2018

I vote 2, 1 if 2 won't go through.

@commy2
Copy link
Contributor

commy2 commented Dec 6, 2018

Voting for vanilla in parallel with CBA.

@commy2 commy2 added this to the 3.9.1 milestone Dec 7, 2018
@commy2 commy2 added the Task label Dec 7, 2018
@commy2 commy2 modified the milestones: 3.9.1, 3.10 Feb 8, 2019
@commy2 commy2 modified the milestones: 3.10, Ongoing Feb 15, 2019
@commy2 commy2 added the WIP label Feb 23, 2019
@commy2
Copy link
Contributor

commy2 commented May 1, 2019

Is this obsolete now? Close?

@dedmen
Copy link
Contributor Author

dedmen commented May 2, 2019

@commy2 yes.
But this
https://github.com/CBATeam/CBA_A3/pull/1030/files#diff-a59084d33a95ff61239e5b86ef372144L8
+= is still needed. See comment on the 1.92 magwell PR. We currently break mods and might break vanilla again with next update.

@commy2 commy2 closed this May 3, 2019
@commy2 commy2 added Invalid and removed WIP labels May 3, 2019
@commy2 commy2 removed this from the Ongoing milestone May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants