Skip to content

Conversation

PaulDemeulenaere
Copy link
Contributor

@PaulDemeulenaere PaulDemeulenaere commented Aug 24, 2021


Purpose of this PR

Fix unexpected public API for VFXHDRPSettingsUtility introduced at #4601


Testing status

Only tested if it compiles locally.
Yamato ⏳


Comments to reviewers

See also this conversation

This class has been recently introduced by #4601
@github-actions github-actions bot added the HDRP label Aug 24, 2021
@Unity-Technologies Unity-Technologies deleted a comment from github-actions bot Aug 24, 2021
@PaulDemeulenaere PaulDemeulenaere marked this pull request as ready for review August 24, 2021 16:42
Copy link
Contributor

@ludovic-theobald ludovic-theobald left a comment

Choose a reason for hiding this comment

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

Hi Paul, thanks for this PR, I also did these changes in that PR, but it's cleaner if it is a PR on its own :)

public static class VFXHDRPSettingsUtility
static class VFXHDRPSettingsUtility
{
public static void RefreshVfxErrorsIfNeeded(ref bool needRefreshVfxErrors)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to set RefreshVfxErrorsIfNeeded() to internal or changing the parent class in enough ?

Copy link
Contributor Author

@PaulDemeulenaere PaulDemeulenaere Aug 25, 2021

Choose a reason for hiding this comment

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

No, otherwise, this function won't be accessible by any user code, we have :

internal class VFXHDRPSettingsUtility
{
 public... //reachable in HDRP
}

@ludovic-theobald ludovic-theobald self-requested a review August 24, 2021 16:49
@sebastienlagarde sebastienlagarde merged commit 73ef535 into master Aug 25, 2021
@sebastienlagarde sebastienlagarde deleted the vfx/fix/remove-unexpected-public-api branch August 25, 2021 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants