Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trial-level configuration #392

Merged
merged 44 commits into from
Dec 9, 2022
Merged

Trial-level configuration #392

merged 44 commits into from
Dec 9, 2022

Conversation

bboudaoud-nv
Copy link
Collaborator

This branch adds support for trial-level configuration inherited from the experiment and session in turn.

This is a first-step towards adding staircase designs to FPSci as described in #390.

Merging this PR closes #391.

@bboudaoud-nv bboudaoud-nv added the enhancement New feature or request label Aug 17, 2022
@bboudaoud-nv bboudaoud-nv self-assigned this Aug 17, 2022
@bboudaoud-nv bboudaoud-nv changed the title Trial level configuration Trial-level configuration Aug 17, 2022
@jspjutNV jspjutNV added this to the Later Release (late 2022) milestone Aug 31, 2022
@bboudaoud-nv bboudaoud-nv marked this pull request as ready for review October 3, 2022 20:25
@bboudaoud-nv
Copy link
Collaborator Author

The bug @jspjutNV mentioned above has now been resolved. Specifically it only occurs when resetPlayerPositionBetweenTrials is false and the view moves away from the default view direction. Along with this change came some fixes for handling getting the correct spawn direction on the first trial of a session when resetPlayerPositionBetweenTrials is false.

@jspjutNV
Copy link
Contributor

I created a pre-release for the current state of this PR. There's still some issues, but this should make it easier for non-developers to test their experiments and report bugs.

@jspjutNV
Copy link
Contributor

I'm noticing a bug in the current version that seems to be related to having a scene name set without a spawnHeading, expecting the default value, or the one set in the scene for the spawn heading. The result is that the dummy target spawns somewhere unreachable in my test, but it may be somewhere reachable in others. Adding a spawnHeading to the config seems to resolve the issue, but is a regression compared to previous versions. We'll want to reflect this change in the documentation and release notes if this change is intentional.

@bboudaoud-nv
Copy link
Collaborator Author

@jspjutNV yes, this bug was introduced by my "fix" for the issue when resetPlayerPositionBetweenTrials is false. I need to think a bit more on all the places the spawn heading might come from and come up with a more robust solution for where to locate the reference target here.

@bboudaoud-nv
Copy link
Collaborator Author

I'm now running into issues with specifying the scene name parameter on a trial-by-trial basis. Will need to look into this configuration interaction.

@bboudaoud-nv
Copy link
Collaborator Author

I believe I've now fixed the issues with scene names changing between trials. @jspjutNV you should be able to test and see if the previous issue with spawnHeading not being specified.

@jspjutNV
Copy link
Contributor

jspjutNV commented Dec 7, 2022

The latest commits fix most of the issues we saw before.

I've notice that 2 tests are failing, but haven't investigated the root cause yet. These failing tests are:

  • FPSciTests.TestAutoFire which fails with the message "Low damage-per-second with auto-fire but target still died"
  • FFPSciTests.TestFreshStart which throws an error on startup (I think it was this one), but when you continue says that a bunch of files did not correctly delete and regenerate. The error messages may be misleading about the root cause here.

@bboudaoud-nv
Copy link
Collaborator Author

bboudaoud-nv commented Dec 7, 2022

@jspjutNV both of these issues are now resolved.

FPSciTests.TestAutoFire was failing as it was still attempting to configure the weapon through the session-level config. I changed this to use the new trial-level config which is what takes effect.
FPSciTests.TestFreshStart was failing due to the m_loadedScene.name not being cleared in FPSciApp::initExperiment() which meant that GApp::loadScene() was being called with an empty string as its argument (as you pointed out this also impacted selection of an experiment through the in-app drop down). I fixed this by clearing the m_loadedScene.name in FPSciApp::initExperiment().

Copy link
Collaborator Author

@bboudaoud-nv bboudaoud-nv left a comment

Choose a reason for hiding this comment

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

I have changes ready to submit for most of these comments, but let me know if I misunderstood the intent of anything you were trying to do here.

data-files/samples/trials.Experiment.Any Outdated Show resolved Hide resolved
data-files/samples/trials.Experiment.Any Outdated Show resolved Hide resolved
data-files/samples/trials.Experiment.Any Outdated Show resolved Hide resolved
data-files/samples/trials.Experiment.Any Show resolved Hide resolved
data-files/samples/trials.Experiment.Any Show resolved Hide resolved
data-files/samples/trials.Experiment.Any Show resolved Hide resolved
data-files/samples/trials.Experiment.Any Show resolved Hide resolved
Copy link
Contributor

@jspjutNV jspjutNV left a comment

Choose a reason for hiding this comment

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

Lots of changes, but I think we got most of the bugs. We'll report and fix any remaining later on.

@jspjutNV jspjutNV merged commit 7c0c472 into master Dec 9, 2022
@bboudaoud-nv bboudaoud-nv mentioned this pull request Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trial-level Configuration
2 participants