med-material / Whack_A_Mole_VR

Whack-A-Mole in VR
MIT License
3 stars 15 forks source link

FadingHelpers.cs class and FadingUtils.cs static class added #219

Closed lucasmnt closed 1 year ago

lucasmnt commented 1 year ago

Theses classes are used to handle different fading effects for different game objects.

Right now these scripts are not used by any game objects since they are subject to (a lot of?) change, but adding them won't do any harm to the existing project.

FadingHelper script can be attached to a game object and will look like this :

image

Here is the purpose of the different parameters :

These scripts can be found in the project hierarchy here : Assets>Scripts>Game.

bastianilso commented 1 year ago

Thanks for uploading this @lucasmnt . It's now more clear to me what you are trying to do. I think we are still talking past each other - and I think we may need to adjust your architecture to be a little simpler. Please take a look at my proposal and let me know what you think. 🙂

FadingUtil: a static class. It is technically designed to allow fading in and fading out for many different objects. Public functions:

FadingUtil.Fade(object Obj, float time, Fade fadeDirection, float fadeDelay=0f, bool setEnable = false)

Now, as for FadingHelper: This is a class in the "classical Unity component" sense. The class is technically designed to be added to game objects via the "Add Component" button, to make applying a fade easier. A convenient "Play Automatically" button makes it possible to fade object automatically, with no need for extra code. It provides following public functions:

FadingHelper.Fade()

The point of this architecture is to 1) make it very clear what the purpose of the classes are and 2) simplify the code architecture and exposure points. And feel free to write up a counter proposal, if you have cases you feel are not currently covered or questions. 🙂

lucasmnt commented 1 year ago

Hello @bastianilso,

I'd like to ask you what is the purpose of "bool setEnable = false" parameter for the Fade function :

FadingUtil.Fade(object Obj, float time, Fade fadeDirection, float fadeDelay=0f, bool setEnable = false)

That being said, I've been working on FadingUtils/Helper classes and i've made good progress. Right now, here what the interface of FadingHelper looks like :

image

Purpose of the different parameters :

Fade in speed and Fade out speed have been removed since we can now precisely ask for an exact duration for the fading effect.

In visual Studio here is the signature of the function in fadingHelper class :

image

As you can see (and as you asked) I called directly the fade function from FadingUtils class, and it is working well. Here is the signature of the Fade( ) function from fadingUtils :

image

The first parameter of this function is a reference of a MonoBehavior object, and is mandatory to have if we want to use Coroutines and IEnumerator in general inside a static class.

bastianilso commented 1 year ago

@lucasmnt nice progress. I think the purpose of the SetEnable parameter, was to allow controlling whether FadingUtil should control the game object's enabled property (through e.g. SetActive(true/false) before or after fading. I'm not sure how exactly this could work, but anyway the point was to make it optional whether we manipulate this or not, since in some cases you may not want the object to be disabled after e.g. a fadeout.

Alternatively, we can just provide a callback parameter and then clients can decide themselves what to do when the fade has finished. This might also be cleaner (e.g. we dont want to make assumptions about what people want to do, who knows maybe someone would even want to destroy an object after fadeout).

bastianilso commented 1 year ago

Try to unify your individual functions by having a function which evaluates what https://github.com/med-material/Whack_A_Mole_VR/blob/master/Assets/LoggingManager/LogStore.cs#L321

image

Can use Debug.LogError() if the user provided something we dont support.

bastianilso commented 1 year ago

@lucasmnt is this ready for review? i still see a lot of code in FadingHelper.cs that I think we discussed moving to FadingUtils.cs

bastianilso commented 1 year ago

summary: think about how to abstract away "how to fade" from "reading alpha value" and "setting alpha value". Think of "reading/setting" as two functions.

then collapse FadeGameObject into just one code for Fade in and one code for Fade out.

lucasmnt commented 1 year ago

All the details here are from the FadingUtils.cs class, and are mainly a direct anwser to your last comment.

FadeRoutine(): Starts a Fade routine

For this part, not much has changed, from FadeRoutine() we fire a coroutine with the FadeGameObject() IEnumerator and all the necessary parameters. Inside the used parameters of the coroutine you can see a call to the function FadeIdentification(Obj) which will return a ComponentToFade enum value that identifies what object are we working with (and that would be a CanvasGroup/Image/SpriteRenderer/MeshRenderer/Text).

image

IEnumerator FadeGameObject: Does the actual fading over a given time, with a given delay, from starting point X to ending point Y..

In this function, I use a switch case depending on the fadeAction parameter that determines if we are working on a fade in effect or a fade out effect, and that will then call the correct function accordingly.

image

The functions that are called are FadeInColor3() and (or) FadeOutColor3() and basically what is happening in these functions is the generalisation of the 5 fade in functions and 5 fade out functions that I had in previous version of my work. These functions use both the new SetAlpha and GetAlpha functions that I will be detailing more below. There is also a last new function called FadeInAlpha (or FadeOutAlpha) that will be acutally used to shift the alpha towards 0 or 1 depending on the need. Here what the functions look like :

image

image

image

SetAlpha(object gameObject, float alphaVal): Function which takes an object, identifies what it is (fx by reading the enum var), and changes its alpha value to X.

For the function SetAlpha(), I use three parameters : the object we are working with, the type of object it is from the enum ComponentToFade, and also the target alpha, which is the value we want the object's alpha changing to. With this I create a switch case that will make it possible to access and change the alpha value of any registered object.

image

ReadAlpha(object gameObject): Function which takes an object, identifies what it is (fx by reading the enum var) and returns its alpha value.

This is basically the same principle as SetAlpha() function, but I use here only two parameters, ComponentToFade enum and the object we are working with, so the function returns the object's aloha value.

image

lucasmnt commented 1 year ago

I made some modifications to the parameters of some functions of the fadingUtils class, and parameters to the fadingHelper class :

image

image

Now, we can pass "On" or "Off" for the setEnable and playAuto parameters, which is more readable than a "true" or "false".

However, I still have a problem with the playAuto parameter and disabling the object, let me explain : we want an object to automatically play its fade effect when it is enabled/disabled. In the use cases what makes sense is fading in an object when it's enabled and fading out an object when it's disabled.

For the enabling part, there is no problem :

image

Using the built-in OnEnable() function from Unity works perfectly fine, allowing us to fade-in an object when we are enabling it, and not necessarily having to call the Fade() function directly. Plus this only happens when autoPlay is set to "On".

The problem comes with disabling the object :

image

Indeed, following the same principle, I wanted to be able to fade out the object without having to actually call for Fade(). The thing is that OnDisable() function plays after the object gets disabled, so how can one fade the object first then disable the object automatically, in the same manner as the fading in case ?

My take on this problem was to make a specific function to call that would do the fading out then disabling, but can we really call it an autoPlay feature since we'll have to call this somewhere in the code ? I mean, we can argue that it is / it is not an autoPlay feature when calling the fading in effect inside OnEnable() but will it make sense ? The thing that I don't like is that the methods and process will not be the same for the two different use cases and i'm afraid that could be confusing : Like why can I just use gameObject.setEnable(true) and have an "auto" fade in effect, but to have my fading out effect + object disabling, why do I have to call explicitly FadingUtils.FadeRoutine() with the correct parameters ?

I hope what i just wrote makes sense to you, because i'm not sure on how to handle this problem (if it is even a problem).

lucasmnt commented 1 year ago

I successfully squashed all the commits into one, awaiting for the merging now :)