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

Implement Better Positive Trait UI #1212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BlackDog86
Copy link
Contributor

Fixes #1081

Summary:

  1. Modified UIAlert to use the "TrainingComplete" alert for positive traits - This has the same set of UI elements as Alert_NegativeSoldierEvent for negative traits but with a neutral colour scheme & promotion-icon.

  2. Updated XcomHQPresentationLayer to check whether traits are positive or negative before calling the notifybanner function (this is the piece which places the trait notifications on the bottom left of the screen). Adjusted colours and icons to make more sense.

  3. Adjusted the UIAvengerHUD OnClickTraitIcon function so that if someone clicks on a soldier walking around the avenger with a negative or positive trait icon (like the bond icon), it takes them to the correct UI Panel for that trait.

Note that I can't confirm this new code actually does anything - I can't remember seeing anyone walking around with trait icons bobbing around so probably the function is never called for, but anyway, the code now includes positive traits as well in case that behaviour is ever fixed in the future.

@RustyDios
Copy link
Contributor

A quick glance, and I fully agree this looks like it fixes a lot of issues there are various places that have for example;

-var() bool							bHasSeenNegativeTraitPopup;
//Issue# 1081 - Alter bool name so it applies to both positive and negative traits
+var() bool							bHasSeenTraitPopup;

Which would break the CHL backwards compatibility policy on any mods previously reliant on the removed bool.
Rather than remove the word 'Negative' from everything, a better approach should be to duplicate the variables adding 'Positive' and make the code account for both.

@robojumper
Copy link
Member

XComGameState_HeadquartersXCom is a native class, so property additions are disallowed. Also in general, keep property names and function names and arguments the same -- even if they have "negativeTrait" in their name, simply keep that and add a comment that this does more than what the name implies.

@BlackDog86
Copy link
Contributor Author

BlackDog86 commented Jun 26, 2023

A quick glance, and I fully agree this looks like it fixes a lot of issues there are various places that have for example;

-var() bool							bHasSeenNegativeTraitPopup;
//Issue# 1081 - Alter bool name so it applies to both positive and negative traits
+var() bool							bHasSeenTraitPopup;

Which would break the CHL backwards compatibility policy on any mods previously reliant on the removed bool. Rather than remove the word 'Negative' from everything, a better approach should be to duplicate the variables adding 'Positive' and make the code account for both.

BC did cross my mind when I changed that - I don't think it matters to just change it back and comment that the function does 'more than what it says on the tin' as it were. I'll modify and re-post. Thanks for the feedback :)

@BlackDog86
Copy link
Contributor Author

Requested changes now implemented (no changes to XComGameState_HeadquartersXCom and function names kept the same for BC)

@Iridar Iridar changed the title Issue# 1081 - Implement Better Positive Trait UI Implement Better Positive Trait UI Jul 1, 2023
@Iridar Iridar added the ready-to-review A pull request is ready to be reviewed label Jul 1, 2023
/// Since Alert_NegativeSoldierEvent is a subclass of Alert_Trainingcomplete in flash, they take the same arguments
/// This allows passing details to the panels using the same .uc code (but generating different outputs)
/// All references to 'negative' traits in the function have been removed but the function name is preserved for BC
simulated function BuildNegativeTraitAcquiredAlert(bool bPositiveTrait)
Copy link
Contributor

Choose a reason for hiding this comment

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

Without looking at anything else, you can't change function definitions of existing functions, that breaks the BC.

The only exception are private/final internal Highlander functions that are explicitly not covered by the BC policy.

Copy link
Contributor Author

@BlackDog86 BlackDog86 Jul 1, 2023

Choose a reason for hiding this comment

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

Understood - I've now reworked the code so that all base game functions are left unmodified. Fairly happy with the code now. Let me know if this is sufficient :)

@Iridar Iridar added waiting-on-author A pull request is waiting on changes from the author and removed ready-to-review A pull request is ready to be reviewed labels Jul 1, 2023
@BlackDog86 BlackDog86 force-pushed the 1081-Positive-Trait-UI branch 2 times, most recently from 42fd62d to 9243878 Compare July 1, 2023 11:00
@Iridar Iridar added ready-to-review A pull request is ready to be reviewed and removed waiting-on-author A pull request is waiting on changes from the author labels Jul 1, 2023
@BlackDog86 BlackDog86 requested a review from Iridar July 1, 2023 15:34
@BlackDog86 BlackDog86 force-pushed the 1081-Positive-Trait-UI branch 4 times, most recently from 2300adb to a1f56fb Compare July 13, 2023 19:18
@Iridar Iridar added this to the 1.27.0 milestone Aug 10, 2023
@Iridar Iridar modified the milestones: 1.27.0, 1.28.0 Oct 29, 2023
@Iridar Iridar modified the milestones: 1.28.0, 1.29.0 May 1, 2024
@BlackDog86
Copy link
Contributor Author

Are there any more changes required on this or is it just a timing thing?

@BlackDog86 BlackDog86 force-pushed the 1081-Positive-Trait-UI branch 2 times, most recently from 92249d9 to 87740df Compare October 29, 2024 00:39
Copy link
Contributor

@Iridar Iridar left a comment

Choose a reason for hiding this comment

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

To be clear, this feature adds just a UI, right? But no way for units to actually acquire positive traits? Docs need to specify how mod authors should take advantage of this feature.

And I'd love to see some screenshots of this UI in action from an example use case.


// Send over to flash
LibraryPanel.MC.BeginFunctionOp("UpdateData");
LibraryPanel.MC.QueueString(m_strSoldierShakenHeader); //ATTENTION
LibraryPanel.MC.BeginFunctionOp("UpdateData");1
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this stray 1 at the end of the line? Would this even compile?

@BlackDog86
Copy link
Contributor Author

BlackDog86 commented Nov 11, 2024

To be clear, this feature adds just a UI, right? But no way for units to actually acquire positive traits? Docs need to specify how mod authors should take advantage of this feature.

And I'd love to see some screenshots of this UI in action from an example use case.

There are pictures of the updated UI in Issue #1081 - not sure how that 1 got in there either - it's probably a stray key-press from when I was fixing commits that had gotten a long way behind the master earlier this month. I'll fix it tonight.

It's been so long since I wrote this PR so I'll have to dig into the details again regarding the documentation, but my understanding is that the 'idea' of positive traits, their templates and the code to create the various alerts existed already but basically positive traits were finally cut from the game on release so the functions were never used.

Anyway, from a user/modder perspective, all they need to do is set bPositiveTrait on the trait template and the UI handling should take care of the rest. I'll run a few other tests later on if rquired.

The mod "More traits" contains the code and config required to add new trait templates to the game - this probably should be considered the de-facto standard for adding new traits - I don't think there are a vast amount of mods that have built many more since then.
https://steamcommunity.com/sharedfiles/filedetails/?id=1122837889

@BlackDog86
Copy link
Contributor Author

Additionally, the base game only allows for one trait popup message to appear at a time. Personally I'd prefer if each trait brought its own popup message when its triggered but I didn't think that that was enough of a justification to comment out the block below. Also, if you use the 'starting traits' mod, it does mean it could bring up quite a few popups when you start a new campaign but I'm open to discussion on leaving the base game code as it is, or adjusting it.

> if(PopupCount > 0 && !XComHQ.bHasSeenNegativeTraitPopup)
> 				{
> 					XComHQ.bHasSeenNegativeTraitPopup = true;
> 					UINegativeTraitAlert(NewGameState, PopupUnit, StoredTraitName);
> 				}

Copy link
Contributor

@Iridar Iridar left a comment

Choose a reason for hiding this comment

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

Looks mostly okay to me, but I have issues with it being a bit messy, like fixing typos or adding random empty spaces to lines.

I'm also nitpicking to docs.

The point of HL-Docs in this case is to be mod author facing, so they can get a clean and concise explanation of what this CHL feature can do for them.

Something along the lines of:

"Positive traits are a scrapped WOTC feature, on release the game had only negative traits, so there is no UI for "positive trait acquired", but X2TraitTemplate does have a bPositiveTrait flag. This feature repurposes "soldier promoted" UI to make it function as a makeshift "positive trait acquired UI". To make the game display it when the soldier acquires a positive trait, simply set bPositiveTrait = true; on your X2TraitTemplate."

This all can be done as one piece of Docs somewhere, and the rest of the changes can be marked with standard non-Docs Start/End Issue tags.

@@ -270,31 +270,31 @@ function UITraitIcon CreateTraitIcon(XComGameState_Unit Unit, bool bPositive)

function OnClickTraitIcon(UITraitIcon Icon)
{
local int UntiID;
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the diligence, but fixing typos in vanilla code is outside the scope of this PR.


UnitState.AlertTraits.RemoveItem(TraitTemplateName);
UnitState.WorldMessageTraits.RemoveItem(TraitTemplateName);

BuildUIAlert(PropertySet, 'eAlert_NegativeTraitAcquired', None, 'OnNegativeTraitAcquired', "Geoscape_CrewMemberLevelledUp", false);
//Begin issue #1081
if (TraitTemplate.bPositiveTrait)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a none-check for the trait template pls.

@Iridar Iridar added waiting-on-author A pull request is waiting on changes from the author and removed ready-to-review A pull request is ready to be reviewed labels Nov 16, 2024
@BlackDog86 BlackDog86 force-pushed the 1081-Positive-Trait-UI branch 2 times, most recently from 1aa75bc to e4672db Compare November 16, 2024 17:00
…its -

This has the same set of UI elements as Alert_NegativeSoldierEvent for negative traits
but with a neutral colour scheme & promotion-like-icon.

Updated XcomHQPresentationLayer to check whether traits are positive or negative
before calling the notifybanner (this is the piece which places the trait notifications
on the bottom left of the screen). Adjusted the icons in the notification area to make
more sense.

Adjusted the UIAvengerHUD OnClickTraitIcon function so if someone clicks on a soldier
with a negative or positive trait, it takes them to the correct UI Panel.
@BlackDog86
Copy link
Contributor Author

OK hopefully this is a bit better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement waiting-on-author A pull request is waiting on changes from the author
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No "gained positive trait" UI
4 participants