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

2026: deprecate robotInit #6845

Closed
spacey-sooty opened this issue Jul 17, 2024 · 12 comments
Closed

2026: deprecate robotInit #6845

spacey-sooty opened this issue Jul 17, 2024 · 12 comments
Milestone

Comments

@spacey-sooty
Copy link
Contributor

No description provided.

@PeterJohnson PeterJohnson added this to the 2026 milestone Jul 17, 2024
@brettle
Copy link
Contributor

brettle commented Jul 18, 2024

Will simulationInit() also be deprecated?

@calcmogul
Copy link
Member

calcmogul commented Jul 18, 2024

simulationInit() can be replicated with an isSimulation() if statement in the constructor, but simulationInit() doesn't feel as redundant as robotInit().

@brettle
Copy link
Contributor

brettle commented Jul 18, 2024

Since you can't override constructors in anonymous subclasses, loss of simulationInit() would preclude the following code:

var testRobot1 = new MyTimedRobot() {
  public void simulationInit() {
    // test-specific robot initialization code
  }
};
assertRobotWorks(testRobot1);

which is convenient for testing. To workaround the loss of simulationInit() in this context one would need to create named subclasses or override simulationPeriodic() to contain some code that disables itself after the first call. Not a huge deal but something to consider.

@Starlight220
Copy link
Member

You can't override constructors, but you can make an initialization block which fulfills basically the same purpose

@brettle
Copy link
Contributor

brettle commented Jul 18, 2024

Fair enough, but an initialization block still isn't quite the same as simulationInit(). simulationInit() is called by TimedRobot.startCompetition() (which in my example would be called during assertRobotWorks()), so it is not run at the same time as a constructor or initialization block.

@calcmogul
Copy link
Member

calcmogul commented Jul 19, 2024

simulationInit() being called in startCompetition() instead of the robot constructor doesn't actually matter. robotInit() is called first in startCompetition(), simulationInit() is called right after that (see here), and startCompetition() is called right after the robot constructor. See #6623 (comment) for more information.

@brettle
Copy link
Contributor

brettle commented Jul 22, 2024

startCompetition() (and thus robotInit()/simulationInit()) is called right after the robot constructor if the robot is run via startRobot(), but in my example above assertRobotWorks() doesn't call startRobot(). It calls startCompetition() with the robot that was constructed earlier. The simulationInit() hook makes it easier to write tests where the simulation initialization needs to change in a way which depends on something that happens between when the robot is constructed and when startCompetition() is called (e.g. something in assertRobotWorks() above). That said, if both robotInit() and simulationInit() are removed, one could achieve the same effect by overriding startCompetition() to run the code that would have been in them and then call super.startCompetition(). So I no longer see a downside to removing simulationInit() when robotInit() is removed.

@calcmogul
Copy link
Member

calcmogul commented Jul 22, 2024

An initializer block as suggested in #6845 (comment) should meet that need. From https://docs.oracle.com/javase/tutorial/java/javaOO/initial.html:

Initializer blocks for instance variables look just like static initializer blocks, but without the static keyword:

{
// whatever code is needed for initialization goes here
}

The Java compiler copies initializer blocks into every constructor. Therefore, this approach can be used to share a block of code between multiple constructors.

Just make a new object for every test with a different initialization block.

@brettle
Copy link
Contributor

brettle commented Jul 22, 2024

Just make a new object for every test with a different initialization block.

While I agree that would often be sufficient, my point was just that an initialization block is not technically equivalent to simulationInit() because other code can be run between when the initialization block is run and when simulationInit() is run. The actual equivalent to simulationInit() (once robotInit() and simulationInit() have been removed) would be code inserted at the beginning of an overriden startCompetition(). Since that equivalent would exist, I'm fine with the removal of robotInit() and simulationInit()).

Just for clarity, as an example of where an initialization block would not be equivalent to simulationInit() and switching to using one might not be preferred, consider:

MyTimedRobot testRobot;

boolean testRequiresSpecialSim;

@BeforeEach
void setup() {
  testRobot = new MyTimedRobot() {
    public void simulationInit() {
      if (testRequiresSpecialSim) {
        initSpecialSim();
      }
    }
  };
}

@Test 
void normalTest() {
  assertRobotWorks();
}

@Test 
void specialTest1() {
  testRequiresSpecialSim = true;
  // ... other test-specific test setup
  assertRobotWorks();
}

@Test 
void specialTest2() {
  testRequiresSpecialSim = true;
  // ... other test-specific test setup
  assertRobotWorks();
}

void assertRobotWorks() {
  testRobot.startCompetition();
  assertGotExpectedResult1();
  // ...
  assertGotExpectedResultN();
}

But again, I'm ok with the removal of robotInit() and simulationInit() because in that case I could get the same result by just changing the setup() method to be:

void setup() {
  testRobot = new MyTimedRobot() {
    public void startCompetition() {
      if (testRequiresSpecialSim) {
        initSpecialSim();
      }
      super.startCompetition();
    }
  };
}

@calcmogul
Copy link
Member

calcmogul commented Jul 22, 2024

I'm actually not sure what test-specific stuff you'd even put in a test-only anonymous class. The simulation stuff like sim counterparts of real objects should already be in the robot class or subsystems.

I also can't think of a case off the top of my head where you'd make the simulation init different. When 3512 unit and integration tested their entire robot in 2020, the tests used the same setters (e.g., pose reset) and getters (e.g., sensor values) the real code did. The only exception was calls to DriverStationSim in the test body itself, because the test was injecting stimuli.

@brettle
Copy link
Contributor

brettle commented Jul 22, 2024

As a specific example, perhaps for the "special" tests we want to use an external virtual world simulator instead of the sims created in the robot class. In that case, initSpecialSim() might be responsible for setting that up, including connecting to the external simulator. But the external simulator might not be started until just before calling startCompetition() in assertRobotWorks().

To be clear, I agree that there are other ways to achieve this. I'm just providing an example of a way that someone might want to organize their code when using simulationInit() that is not equivalent to having that code in an initialization block. Hope that helps.

@calcmogul
Copy link
Member

Duplicate of #6622.

@calcmogul calcmogul closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2024
sciencewhiz pushed a commit to wpilibsuite/RobotBuilder that referenced this issue Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants