med-material / Whack_A_Mole_VR

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

Fix bug with MonoBehaviour inheritance #220

Closed Xav1204 closed 1 year ago

Xav1204 commented 1 year ago

During my tests of the steamvr version upgrade, I've found that the event log inside or outside motorspaces didn't work. So, I found the problem and I fixed it.

During my last tests to write this kind of logs in the csv file, I probably opened an old files with the events wrote inside and where the inheritance class was Pointer and not MonoBehaviour.

I've also changed the log event name. It's maybe clearer now.

I referred to the LaserMapper class to create the log system in the csv.

image

bastianilso commented 1 year ago

@Xav1204 please be more precise in your descriptions of your work in the pull request. I can't read your mind. 🙂

  1. Please explain what the problem is (why it doesnt work). (saying "So, I found the problem and I fixed it." does not explain to us what the technical problem was).

  2. Please explain the rationale for why you now use LoggingManager instead of loggerNotifier. (you are not mentioning this change at all).

  3. Explain how you have changed the log event name. (writing that you did so, does not make it clear how it changed, you just put extra work on others to look all this up).

Xav1204 commented 1 year ago

@bastianilso

  1. The problem is that the event "pointer inside motorspace" and "pointer outside motorspace" not appeared in the csv file when we use private LoggerNotifier. As I said, during my tests I probably opened a wrong csv file and I thought that it worked but it's not the case.

  2. As I didn't know why is the problem, I researched in others files in the same folder if a MonoBehaviour inheritance class that logs datas existed. It was true, the LaserMapper class is a Monobehaviour inheritance class and log datas in the csv file. So, I took the function in LaserMapper to adapt it to our BubbleDisplay class. After my test, I saw that it worked and as we use this function in LaserMapper, I thought that there won't be any problem to use it in BubbleDisplay class because each classes use the cursor to collect specific datas.

LaserMapper function: image

BubbleDisplay function: image

In these functions, we define the Headers where we want to write the data and the data after the ','. The loggingManager.Log("Event") refers to the csv file in which we want to write the datas.

3.In order to change the event type, we need to use the second line in the function that define the name of the EventType. We have two options to choose the EventType. In first, we can take it in an enum that contains all kind of event type or like in LaserMapper class, we can define it directly with a string variable.

I hope that it's clearer for you now. Please let me know if it's a problem for you to use the same function that LaserMapper class to log BubbleDisplay class datas.

Xav1204 commented 1 year ago

@bastianilso I've made some test in order to know how the loggerNotifier works.

  1. First, if we use the class Inheritance Pointer, as Pointer class has a loggerNotifier dictionary, we don't need to create one in the BubbleDisplay class.
  2. Like the LaserMapper class, we can have a reference to LoggingManager class and its function Save that will save our logs in the right file.
  3. If we create a dictionary in the function Start in the BubbleDisplay script, the log will be written in the event log csv file. However we will have a specific column to stock the datas but as you said during the last issues discussions about the kind of logs, we don't really need to have data about this event.

So for me, as BubbleDisplay and LaserMapper are similar because they work on events in the motorspace, use the second case it's maybe the most logical as we use this one in LasserMapper class.

Please, let me know what you want to do for this class.

Xav1204 commented 1 year ago

@bastianilso it's also good for this pull request.