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

Change ace_map settings to use CBA init directly #5984

Merged
merged 6 commits into from
Jan 28, 2018

Conversation

mharis001
Copy link
Member

@mharis001 mharis001 commented Dec 28, 2017

When merged this pull request will:

  • Convert all ace_map settings to CBA settings directly
  • Give BFT settings their own category
  • Make default channel setting a list

@PabstMirror PabstMirror added this to the Ongoing milestone Dec 28, 2017
@PabstMirror PabstMirror added kind/cleanup Release Notes: **CHANGED:** kind/enhancement Release Notes: **IMPROVED:** labels Dec 28, 2017
@jonpas
Copy link
Member

jonpas commented Dec 28, 2017

I think this is perfect to change the rest of the settings in this component.

@PabstMirror should we remove reading the setting from module (call to the function that applies it)?

@mharis001
Copy link
Member Author

@jonpas I can convert the settings in map component if that is decided

@jonpas
Copy link
Member

jonpas commented Dec 28, 2017

@mharis001 thank you! Let's wait to hear from others first.

Copy link
Member

@kymckay kymckay left a comment

Choose a reason for hiding this comment

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

Would happily merge as is, but also feel free to convert all the settings in this component and we can re-review (I don't see any reason not to - we've already got a few minor settings converted to CBA).

@mharis001
Copy link
Member Author

mharis001 commented Dec 29, 2017

Should BFT be its own component, or maybe a more generalized unit tracking?

@commy2
Copy link
Contributor

commy2 commented Dec 30, 2017

I think the BFT settings should be in their own Category.

@mharis001 mharis001 changed the title Make default channel setting a list Change ace_map settings to use CBA init directly Dec 31, 2017
@mharis001
Copy link
Member Author

Remove question mark at the end of displayName of BFT_ShowPlayerNames and BFT_HideAiGroups?

@commy2
Copy link
Contributor

commy2 commented Dec 31, 2017

YES! Please do that! Also remove the : if present. Some other settings use that and it also doesn't belong.
Also worth looking into how to capitalize the English.

@mharis001
Copy link
Member Author

I think sentence case would be better than title case for settings.

@PabstMirror
Copy link
Contributor

Take a look at https://github.com/acemod/ACE3/pull/5581/files#diff-e1cf3ccb88bb6158f0e98e8cbd4f9483R4

We'll want to leave the ace_settings in, but empty except for movedToSQF = 1;

(Because we need to know at preInit if a mission config setting is a change, or a brand new setting for the mission)

@mharis001
Copy link
Member Author

mharis001 commented Jan 1, 2018

Thats my bad, i'll fix it. Do you want to also place all settings in a initSettings.sqf file?

@jonpas
Copy link
Member

jonpas commented Jan 1, 2018

Do you want to also place all settings in a initSettings.sqf file?

Yes, please.

Copy link
Member

@kymckay kymckay left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Will let someone else give a second review and merge just to make sure I've not missed anything

@PabstMirror
Copy link
Contributor

We could keep BFT stuff in same category and just put them in a sub category
image

CBA 3.6 isn't out yet, so we can always do this in a later PR
[format ["ACE %1", localize LSTRING(Module_DisplayName)], localize LSTRING(BFT_Module_DisplayName)],

@PabstMirror PabstMirror merged commit b30e188 into acemod:master Jan 28, 2018
@mharis001
Copy link
Member Author

Sub category sounds good once CBA 3.6 is released.

@PabstMirror PabstMirror modified the milestones: 3.13.0, 3.12.2 Apr 2, 2018
BaerMitUmlaut pushed a commit that referenced this pull request Aug 5, 2019
* Make default channel setting a list

* Convert more map settings to CBA

* Give BFT settings own category

* Remove '?' from end of setting display names

* Fix mistake of ACE_Settings removal (movedToSQF = 1)

* Move settings to initSettings.sqf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Release Notes: **CHANGED:** kind/enhancement Release Notes: **IMPROVED:**
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants