St/newbuffer #49
St/newbuffer #49
Conversation
|
I thought of a couple of improvements overnight, just trying to tidy this up with clearer function names and inheritance, so converted to draft. |
…clearer function names, and the three classes used provide minimal intefaces and clearer documentation.
|
Ready for review again. Hopefully clearer. |
|
Thanks @thompson318 - I should have time to review this tomorrow 👍 |
K-Meech
left a comment
K-Meech
left a comment
There was a problem hiding this comment.
Thanks @thompson318 - great to have everything under the MIT license again! 😄
I've put some comments below. The main one is that I think you might need to separate the speed / data steady logic into separate classes. At the moment they can be called with incompatible data due to the differences in what GetData returns.
| using System.Linq; | ||
| using RingBuffer; | ||
| using Tobii.GameIntegration.Net; | ||
| using UnityEngine; |
There was a problem hiding this comment.
| using UnityEngine; |
I don't think this import is used?
| private int minDataRequired; | ||
| protected T[] buffer; | ||
|
|
||
| public TobiiBuffer(int capacity, int minDataRequired) |
There was a problem hiding this comment.
Maybe add definition of capacity / minDataRequired params? Would be useful to know what these should be set to if creating a new buffer from scratch
| ) | ||
| hasData = true; | ||
| int newIndex = lastAddedIndex + 1; | ||
| if (newIndex >= minDataRequired) |
There was a problem hiding this comment.
I think you need a different check here. For example, say I set my minDataRequired to 2 items.
If I'm currently inserting my second item into the buffer, my index will be 1 and this check will fail (even though I do have enough data)
It fails more explicitly, if I have already filled the buffer and I'm now overwriting items. For example, say my buffer has a capacity of 5 and it is already full + I have overwritten the item at index=0. When I go to insert an item at index=1, this check will fail and say I don't have enough data (even though the entire buffer is full)
| /// float vector of the form [time, position]. | ||
| /// </summary> | ||
| /// <param name="speedTime">The time period in seconds over which to calculate the average speed.</param> | ||
| /// <returns>The average speed of the head pose buffer over the given time period.</returns> |
There was a problem hiding this comment.
| /// <returns>The average speed of the head pose buffer over the given time period.</returns> | |
| /// <returns>The average speed of the buffer over the given time period.</returns> |
| /// Calculates the average speed of the buffer over a given time period. Speed is | ||
| /// calculated to the average change in position of the second array returned by GetData | ||
| /// Divided by the total change in the first array returned by GetData | ||
| /// This presumes that the GetData interface of the templated object returns a | ||
| /// float vector of the form [time, position]. |
There was a problem hiding this comment.
| /// Calculates the average speed of the buffer over a given time period. Speed is | |
| /// calculated to the average change in position of the second array returned by GetData | |
| /// Divided by the total change in the first array returned by GetData | |
| /// This presumes that the GetData interface of the templated object returns a | |
| /// float vector of the form [time, position]. | |
| /// Calculates the average speed of the buffer over a given time period. | |
| /// Speed is calculated as the average change in position of the second array returned by GetData | |
| /// divided by the total change in the first array returned by GetData. | |
| /// This presumes that the GetData interface of the templated object returns a | |
| /// float vector of the form [time, position]. |
| /// <summary> | ||
| /// Wrapper for Tobii head pose yaw data, implementing GetData and timestamp interfaces. | ||
| /// </summary> | ||
| class RocketHeadYaw : ITimeStampMicroSeconds, IBufferData |
There was a problem hiding this comment.
| class RocketHeadYaw : ITimeStampMicroSeconds, IBufferData | |
| class HeadYaw : ITimeStampMicroSeconds, IBufferData |
I'd probably avoid having Rocket in any of the class names in this file? We may need to re-use buffers for the gaze / head position / head rotation in other games (e.g. potentially to add smoothing, or for the adaptive difficulty - potentially star collector may need to rely on some speed measurements)
| /// <param name="tolerance">the allowable range</param> | ||
| /// <param name="targetPoint_0">the target point for the first item returned by GetData</param> | ||
| /// <param name="targetPoint_1">the target point for the second item returned by GetData</param> | ||
| protected bool dataSteady(float time, float tolerance, float targetPoint_0, float targetPoint_1) |
There was a problem hiding this comment.
I'm not sure about having all the speed / dataSteady implementations under the base TobiiBuffer class? For example, getSpeed assumes the GetData interface returns a float vector of the form [time, position], which isn't true for the RocketGazePoint class. Equally, dataSteady is assuming that it is returning [x, y] which isn't true for the RocketHeadPitch and RocketHeadYaw classes.
I think potentially you need two separate buffer classes - one for the gaze point and one for head angles, that separates the speed / data steady functionality between them (so they can only be called in a context where the data matches what these methods expect).
I had a very quick hack around on a branch to give an example of what I was thinking (docstrings not updated etc): alternate trackerbuffers. You could still have TobiiBuffer as the base, but this only contains functionality for adding new items, and retrieving items within the last n seconds (see very rough example on that branch). Then the head angle / gaze point buffers could inherit from this adding speed and data steady functionality respectively. What do you think? No worries if this is adding too much complexity
There was a problem hiding this comment.
I like the look of your refactor, thank you. I wanted to something similar but got stuck on the CopyToTwoArrays function needing to access both timestamps and data. I didn't think to separate the time cut off logic from the data copy logic, which now you've done it seems obvious. I'll have a go at merging your branch into this one then getting things working again. May not be this week though. I don't think this is blocking anything is it?
There was a problem hiding this comment.
Thanks @thompson318 - glad that it's useful! No rush on this - it's not blocking anything, so whenever you have time.
This addresses #44, allowing us to return to MIT License.
It implements a bare minimum replacement for the functionality we used in RingBuffer.cs (which was very little) then derived classes for gaze, pitch, and yaw.