Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[PS] Display fade overlay for locked skins #63

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from

Conversation

moinm3uw
Copy link
Owner

@moinm3uw moinm3uw commented Mar 12, 2025

@moinm3uw moinm3uw requested a review from JanSeliv March 12, 2025 18:48
@moinm3uw moinm3uw changed the base branch from main to develop March 12, 2025 18:48
moinm3uw added 10 commits March 16, 2025 04:05
* refactoring

* renamed the properties for shorter names

* drastically improved comments

* fixed code style issues

* code style updates

* made const elements

* simplified if statement

* added pointer handle

* added additional state for the FadeOut to return early, renamed one more time variable for better naming

* simplified same verification

* improved naming to be clear

* fixed ticket with state none

* fixed issue with skins get unlocked accidentally

* added ability to have instant switch for skin change but keep animation for plat change
// find last unlocked skin
for (int32 Count = CurrentSkinIndex; Count >= 0; Count--)
{
bool bIsLastUnlockedFound = Count > 0 ? MeshComp.IsSkinAvailable(Count) : bIsCurrentSkinAvailable = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. The main problem here: assigning = true at the end of ternary is mistake, you probably wanted the equality == operator instead, but mistakenly used assigning =
  2. Please dont forget about const

Expected Result

Where I additionally further simplified the line by replacing ternary with simple condition, that is the same:

const bool bIsLastUnlockedFound = Count > 0 && MeshComp.IsSkinAvailable(Count) || bIsCurrentSkinAvailable;

Copy link
Owner Author

@moinm3uw moinm3uw Mar 20, 2025

Choose a reason for hiding this comment

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

here I need to assign bIsCurrentSkinAvailable to true and then assign bIsLastUnlcokedUnlockedFound.
I can simplify reading this to something like:

Suggested change
bool bIsLastUnlockedFound = Count > 0 ? MeshComp.IsSkinAvailable(Count) : bIsCurrentSkinAvailable = true;
bool bIsLastUnlockedFound = false;
if (Count > 0)
{
bIsLastUnlockedFound = MeshComp.IsSkinAvailable(Count);
}
else
{
bIsCurrentSkinAvailable = true;
bIsLastUnlockedFound = bIsCurrentSkinAvailable;
}

Let me know if I need to incorporate those changes.
by far const added as part of c1a8d9d

Copy link
Collaborator

Choose a reason for hiding this comment

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

@moinm3uw, current logic still looks overcomplicated. Please simplify it by removing the unused bIsCurrentSkinAvailable variable entirely, avoid assignments inside ternary, and replace the ternary with a clean inline condition using logical operators. This will improve clarity and prevent side effects that may confuse future readers.

Next simplified logic should do all the same as original:

// Skip if current skin is already available
if (MeshComp.IsSkinAvailable(CurrentSkinIndex))
{
	return;
}

// Find last available skin from current index downward
for (int32 Count = CurrentSkinIndex; Count >= 0; Count--)
{
	if (Count == 0 || MeshComp.IsSkinAvailable(Count))
	{
		MeshComp.ApplySkinByIndex(Count);

		constexpr bool bApplySkin = true;
		RefreshAmountOfUnlockedSkins(bApplySkin);
		break;
	}
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

done 07bfc89

@moinm3uw
Copy link
Owner Author

recovered references after refactoring:
image
image

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants