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

Garage Sell Button #3237

Merged

Conversation

Tiny-DM
Copy link

@Tiny-DM Tiny-DM commented Apr 21, 2024

What type of PR is this.

  1. Bug
  2. Change
  3. Enhancement

What have you changed and why?

Information: Added an integrated sell button to the garage GUI.
Checks for commander status and locked status before selling

Please specify which Issue this PR Resolves.

closes #2377

Please verify the following and ensure all checks are completed.

  1. Have you loaded the mission in LAN host?
  2. Have you loaded the mission on a dedicated server?

Is further testing or are further changes required?

  1. No
  2. Yes (Please provide further detail below.)

How can the changes be tested?

Steps:
Mostly needs testing to see if everything works in a multiplayer setting. None of the core functionality was changed too much, so it should be fine but still needs to be tested


Notes:

@Tiny-DM Tiny-DM added Enhancement New feature or request Review pending labels Apr 21, 2024
@Tiny-DM Tiny-DM added this to the 3.6 milestone Apr 21, 2024
@Tiny-DM Tiny-DM requested review from jaj22 and HakonRydland April 21, 2024 19:33
@jaj22
Copy link

jaj22 commented Apr 23, 2024

Mostly copied from #general-dev-stuff:

The main thing you're missing is that the internal garage code shouldn't be calling Antistasi functions (eg resources, vehiclePrice) or using Antistasi vars (eg. theBoss) directly. Instead that should go through stub functions in public.inc, and selling ability shouldn't be assumed, because the garage might be used as a separate mod where money isn't a valid concept.

You'll need a separate commander check function from isCmdClient because unlocking and selling permissions are somewhat different. Should rename isCmdClient to canLock or something similarly specific.

@jaj22 jaj22 added Change requested A change has been requested, and this can't be merged until it's done. and removed Review pending labels Apr 23, 2024
@Tiny-DM Tiny-DM added Review pending and removed Change requested A change has been requested, and this can't be merged until it's done. labels Apr 28, 2024
Copy link

@jaj22 jaj22 left a comment

Choose a reason for hiding this comment

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

Just dealing with the Antistasi/Garage split for now.

A3A/addons/garage/Core/fn_sellVehGRG.sqf Outdated Show resolved Hide resolved
A3A/addons/garage/Core/fn_getVehicleSellPrice.sqf Outdated Show resolved Hide resolved
A3A/addons/garage/Public/config.inc Outdated Show resolved Hide resolved
A3A/addons/garage/Public/config.inc Outdated Show resolved Hide resolved
@wersal454
Copy link

Is editing garage code legal? Or you asked Hakon for it?

@jaj22
Copy link

jaj22 commented Apr 30, 2024

Is editing garage code legal? Or you asked Hakon for it?

Asked Hakon, yes.

@Tiny-DM Tiny-DM requested a review from jaj22 May 4, 2024 18:15
Copy link

@rautamiekka rautamiekka left a comment

Choose a reason for hiding this comment

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

Consistency, a spelling error, and an optimization.

A3A/addons/core/functions/Base/fn_getVehicleSellPrice.sqf Outdated Show resolved Hide resolved
A3A/addons/core/functions/Base/fn_getVehicleSellPrice.sqf Outdated Show resolved Hide resolved
A3A/addons/garage/Core/fn_sellVehGRGLocal.sqf Outdated Show resolved Hide resolved
Co-authored-by: Jouni Järvinen <[email protected]>
Copy link

@jaj22 jaj22 left a comment

Choose a reason for hiding this comment

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

This is fine. Could be refactored a bit, but that wouldn't necessarily make it more readable or substantially reduce the code size. I think I'd also prefer the sell price on the button, but whatever.

Marked one chunk of dead code that can be cleared out.

Comment on lines 29 to 35
private _disp = findDisplay HR_GRG_IDD_Garage;

{
_ctrl = _disp displayCtrl _x;
_ctrl ctrlEnable false;
_ctrl ctrlShow false;
} forEach [HR_GRG_IDC_ExtraMounts,HR_GRG_IDC_ExtraTexture,HR_GRG_IDC_ExtraAnim,HR_GRG_IDC_ExtraPylonsContainer];
Copy link

Choose a reason for hiding this comment

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

This doesn't do anything and can be stripped out. It's only present in onLoad because these controls all start enabled, and switchExtrasMenu only disables the currently-enabled control for some reason.

Copy link

@rautamiekka rautamiekka left a comment

Choose a reason for hiding this comment

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

Another consistency.



Return Value:
<nil>

Choose a reason for hiding this comment

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

Suggested change
<nil>
<NIL>

I missed this one somehow.

@Bob-Murphy Bob-Murphy merged commit 6ce11a7 into official-antistasi-community:unstable May 17, 2024
@Bob-Murphy Bob-Murphy added the Added to changelog Added to changelog label Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Added to changelog Added to changelog Enhancement New feature or request Ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add a sell button to the garage
5 participants