BF: Fix Builder getting start/stop times when using non-slip timing

[mh105]

Three things in this PR:

1. BF of the other half of the bug during BF: starting and stopping times for EyetrackerRecordComponent #6607 by 859a93a that I missed during BF: don't change self.param in getStartAndDuration of et component #6615. Small scale bug that has only affected EyetrackerRecordComponent so far, but it's good to fix for the future.

2. There is a misleading function header in BaseComponent.getStartAndDuration() that hints that the method outputs do not affect stimulus presentation, which is not actually accurate when nonSlipSafe==True. This is especially a confusing problem due to recent NF: Add "force nonslip" param to Routine Settings #6603 that adds the forceNonSlip option to Routine setting. I've updated the method header to make it very clear how is nonSlip timing used downstream to prevent future bugs to get introduced here (I'm especially worried about future component subclasses overriding this method).

3. Right now it's hard to see components with FOREVER duration at the end of a routine because they are no longer adding some minimal duration to make it visible (updated in 26f862d, which is the right thing to change otherwise forceNonSlip won't work correctly). So I've made a QoL change to add back 1s minimal duration to visualize components better on the timeline, unless forceNonSlip is True, in which case there's probably a very good reason to do so, and the person can live with sub-optimal display.

[mh105]

@TEParsons Not sure that I can request reviews through Gh, but let me know if you have any concerns or how I should improve this to get merged!

[TEParsons]

Honestly it's mostly just me wanting to be sure as non-slip timing is a bit of a brain breaker at the best of times!

[TEParsons]

This is the part I'm least sure about - wouldn't that then affect how Components start and stop in actual experiments?

[mh105]

That is a great question and the same one I had when I first saw you changed it from 1 to 0 in 26f862d, so I looked at every call of Routine.getMaxTime() in PsychoPy, and my conclusion is that it used to not affect, but when you added the ['forceNonSlip'] option, it now could. This is why I used an if-else statement here, because when self.settings.params['forceNonSlip'] is True, duration needs to be 0 since otherwise it will affect how Components start and stop in actual experiments.

Longer explanation below FYI.

1. Routine.getMaxTime() is first used twice in psychopy/app/builder/builder.py when drawing the canvas. It's here and here. This is actually the use case for why I needed to set duration to 1 unless ['forceNonSlip'] is used because otherwise a component without end time doesn't display well on the time frame. Anyways, since this module only controls the builder canvas, it doesn't interact with actual experiment flow and it's safe here.

2. Routine.getMaxTime() is also used 6 times in psychopy/experiment/routines/_base.py to control how builder generated scripts are written. This use case indeed involves how components and routines start and stop, except it used to be strictly gated by the nonSlipSafe boolean flag. I will list all 6 instances out here for completeness.

• Here: they aren't even used. This line should have been removed in the first place but I didn't want to complicate the PR.
• Here: you can see below that the maxTime is only used when useNonSlip is True. So when a component doesn't have an end time AND self.settings.params['forceNonSlip'] doesn't flip nonSlipSafe back to True, the output maxTime is not used here in writeMainCode().
• Here: same as above, just for JS.
• Here: the maxTime isn't used in this function and should have been replaced by _.
• Here: as above, the maxTime isn't used in this function and should have been replaced by _.
• Here: This is for JS code, but the use of maxTime is also gated by useNonSlip. So as long as we don't have something forcing a routine as safe for non-slip timing, but we do ;), it shouldn't impact actual experiment timing.

So in summary, yes the maxTime outputted by Routine.getMaxTime() doesn't normally impact experiment timing when nonSlipSafe=False. But since you added the option to force non-slip timing, we just need to set this duration to 0 when user requested for forced non-slip timing. Like I said in the PR message:

I've made a QoL change to add back 1s minimal duration to visualize components better on the timeline, unless forceNonSlip is True, in which case there's probably a very good reason to do so, and the person can live with sub-optimal display.

I think this is a reasonable compromise between visual display and experiment control... those who wanted to force non-slip timing (although given my past experience with fMRI and EEG should never really happen), they can live with a bit of suboptimal drawing of the component on canvas in builder 🤓.

I hope this clarifies this point!

[TEParsons]

I think I follow and yes I agree, not being able to see the stop time in Builder isn't a huge deal as it's still available via the Component dialog, whereas getting an incorrect stop time in-experiment when using non-slip timing would be a big problem, so the compromise you suggest is a good one.

In general I think having one function getMaxTime for both drawing the Routine and writing its code is a bad idea, we should move to having separate functions for each to avoid UI workarounds slipping into actual experiment code. Maybe in the dev branch we could rework getMaxTime to return purely accurate experiment-safe values, ignoring estimated times altogether, and have a second function (getMaxTimeForDisplay maybe?) which uses the values from getMaxTime alongside estimated values and with fallbacks (like replacing 0 to make it visible) to work out the best times to display in Builder. I'll tag you in the PR if I get something working :)

[mh105]

Oh yeah definitely. Having separate functions would be much clearer... I did have to sit and concentrate for quite a bit to sort through the current convoluted working solution, which is in a delicate balance :)