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

Integrate Facility-Clearing-UI-Indicator fix into CHL #1208

Conversation

BlackDog86
Copy link
Contributor

Fixes #1181

Added UIFacilityGrid_FacilityOverlay & replaced appropriate code with that from the Clearing-UI-Indicator fix.

Tested & working as expected - engineer staffed in second excavation slot appears in the second slot on the UI.

@BlackDog86 BlackDog86 changed the title Issue# 1181 Issue# 1181 - Integrate Facility-Clearing-UI-Indicator fix into CHL Jun 25, 2023
@Iridar Iridar added bug-basegame waiting-on-author A pull request is waiting on changes from the author labels Jun 25, 2023
@Iridar Iridar added this to the 1.26.0 milestone Jun 25, 2023
@BlackDog86
Copy link
Contributor Author

Anything else needed on this one?

Tuple.Data[7].kind = XComLWTVBool; // Label autosize
Tuple.Data[7].b = false;

`XEVENTMGR.TriggerEvent('FacilityUICustomizeHook', Tuple, Room);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this even has to do with fixing the issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah those are just hooks from the original mod to allow over-riding some facility status labels. Not important at all, will remove.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are integrating the mod, then surely NOT including these hooks could break any mods dependant on them from the original mod ??

Copy link
Contributor

Choose a reason for hiding this comment

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

We're integrating the bugfix, not the mod.

@@ -1339,26 +1375,23 @@ function array<EUIStaffIconType> GetStaffIconData(XComGameState_FacilityXCom Fac
return NewIcons;
}

// Issue #1181 - Replace original GetStaffIconDataForClearingSlots function with the same from Facility UI Indicator Fix
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should address the specific issue you're fixing.

NewIcons.AddItem(eUIFG_EngineerEmpty);
if (Room.GetBuildSlot(i).IsSlotFilled())
NewIcons.AddItem(eUIFG_Engineer);
else
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do it like this:

function array<EUIStaffIconType> GetStaffIconDataForClearingSlots()
{
	local int i;
	local array<EUIStaffIconType> NewIcons;
	local XComGameState_HeadquartersRoom Room;
	
	// Variable for Issue #1181 
	local XComGameState_StaffSlot BuildSlot;

	Room = GetRoom();

	// Start Issue #1181
	/// HL-Docs: ref:Bugfixes; issue:1181
	/// Make facility staff icons correctly display which staff slots are filled.
	if (Room != none)
	{
		for (i = 0; i < Room.BuildSlots.Length; ++i)
		{
			BuildSlot = Room.GetBuildSlot(i);
			if (BuildSlot == none)
				continue;
			
			if (BuildSlot.IsSlotFilled())
			{
				NewIcons.AddItem(eUIFG_Engineer);
			}
			else
			{
				NewIcons.AddItem(eUIFG_EngineerEmpty);
			}
		}
	}
	// End Issue #1181
	
	//------------------------------------------------
	return NewIcons;
}

With indentation, brackets and comments.

@Iridar
Copy link
Contributor

Iridar commented Jun 26, 2023

Anything else needed on this one?

Apparently forgot to click the button to finish the review and my messages weren't visible to you.

@Iridar Iridar changed the title Issue# 1181 - Integrate Facility-Clearing-UI-Indicator fix into CHL Integrate Facility-Clearing-UI-Indicator fix into CHL Jun 29, 2023
@BlackDog86 BlackDog86 requested a review from Iridar July 1, 2023 15:32
@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 3, 2023
@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 Aug 10, 2023
@Iridar
Copy link
Contributor

Iridar commented Aug 10, 2023

Still has unaddressed issues since the last review.

…acility UI Indicator Fix (i.e. Display correctly which facility slots are empty)
@BlackDog86 BlackDog86 force-pushed the 1181-Facility-Clearing-UI-Indicator-Fix branch from f31b119 to 8235f0e Compare August 12, 2023 10:25
@BlackDog86 BlackDog86 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 Aug 12, 2023
@BlackDog86
Copy link
Contributor Author

Updated & tested. Should be ready for merge.

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 sane, gonna merge after testing.

@Iridar Iridar added ready-for-merge the pull request was reviewed and is ready to be merged. and removed ready-to-review A pull request is ready to be reviewed labels Aug 13, 2023
@Iridar
Copy link
Contributor

Iridar commented Aug 13, 2023

Tested, works fine. Good work.

@Iridar Iridar merged commit d3cd920 into X2CommunityCore:master Aug 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-basegame ready-for-merge the pull request was reviewed and is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate Facility Clearing UI Indicator Fix
3 participants