separate examples from tests

[stephengold]

Plenty of apps in the jme3-examples subproject are actually tests that haven't been automated. Examples, on the other hand, are apps that illustrate how to access certain features of the Engine.

Meanwhile, the package names in jme3-examples all begin with jme3test. and the chooser app is named TestChooser. Mixing tests and examples in a single subproject has been confusing novice users of JME for a long time.

Tests which can be automated should be. Those that can't should be moved to a new subproject jme3-tests. jme3-examples would then be reserved for tutorial code, demos, sample apps, and the like. Its chooser app would be renamed, and all its source files moved to new jme3example packages.

[louhy]

Been on board with this idea for a while. Would you like me to take a look at this and propose a restructuring plan on the forum? Will take some time sorting out what belongs where and settling on other changes like renaming.

Also, I suggest considering the test-automation part of this a separate task, to keep the scope small (I'm not sure if I'd find the time to get that far in one go).

[stephengold]

Fine with me.

[louhy]

I'm not sure I'm going to get to this in a reasonable time frame after all... If someone else wants to go ahead with it, feel free. Hopefully the questions and discussion I started on this thread will help to clarify goals for anyone who does:
https://hub.jmonkeyengine.org/t/splitting-examples-from-tests/42890

[stephengold]

A good start would be to automate test apps that don't require a visual component. For instance, the following apps could be automated and moved to jme3-jbullet/src/test/java:

• TestIssue1004
• TestIssue889
• TestIssue931
• TestIssue970

The following apps could be automated and moved to jme3-core/src/test/java:

• TestIssue1138
• TestCloner
• TestCloneSpatial
• TestIDList
• TestTempVars
• TestIssue1421
• TestAbsoluteLocators
• TestManyLocators
• TestCustomLoader
• TestRayCollision
• TestRayCasting
• TestHalfFloat

[MeFisto94]

One finding while working on TestIssue1004: In order to load models like sindbad, we need the full jme3-desktop dependency and an AWT Context, because AWT is used as our primary texture loader.

I have to see how difficult/important that for Github Actions is, but maybe we want to improve other texture loaders in the future for the sake of portability (e.g. I think Riccardo added stb_image.h as native loader for android) or also just have textureless models for unit testing

[stephengold]

I don't see any major difficulty there.

[MeFisto94]

Indeed, I could solve that, I still need to check it on an actual headless environment, but it should work like that.
I however think I found an issue, related to #889, which probably won't be covered by the test itself:

        if (removed) {
            PhysicsSpace.getPhysicsSpace().add(this);
        }

The above snippet stems from RBC#rebuildRigidBody which is called from setSpatial, i.e. the inverse to addControl().
This only triggers when the RBC was previously added to the scene, but will then also use the Thread Local Physics Space, ignoring the local physicsSpace value.

Furthermore, in PhysicsSpace#addCollisionObject:

} else if (obj instanceof PhysicsVehicle) {

is redundant, because PhysicsVehicle already extends PhysicsRigidBody and so is already added

[stephengold]

will then also use the Thread Local Physics Space, ignoring the local physicsSpace value.

Are you saying that the thread-local physics space might be different from the RBC's physics space? I don't see how...

redundant, because PhysicsVehicle already extends PhysicsRigidBody and so is already added

I don't think this is redundant. A vehicle needs some additional setup that a non-vehicle PRB doesn't.

[MeFisto94]

I don't think this is redundant. A vehicle needs some additional setup that a non-vehicle PRB doesn't.

Well it is, as my IDE told me, take a look here:

jmonkeyengine/jme3-jbullet/src/main/java/com/jme3/bullet/PhysicsSpace.java

Lines 453 to 463 in ce706e5

	public void addCollisionObject(PhysicsCollisionObject obj) {
	if (obj instanceof PhysicsGhostObject) {
	addGhostObject((PhysicsGhostObject) obj);
	} else if (obj instanceof PhysicsRigidBody) {
	addRigidBody((PhysicsRigidBody) obj);
	} else if (obj instanceof PhysicsVehicle) {
	addRigidBody((PhysicsVehicle) obj);
	} else if (obj instanceof PhysicsCharacter) {
	addCharacter((PhysicsCharacter) obj);
	}
	}

Since PhysicsVehicle extends PhysicsRigidBody, the branch at L457 is taken.
Furthermore there's only one private void addRigidBody(PhysicsRigidBody node), so even if it wasn't so, the physics space would execute the same add method.

Actually, I just see, that inside addRigidBody there is an additional instanceof check for PhysicsVehicle, so it just happens further down the road internally, and so the earlier checks are not required (anymore? atm?)

Are you saying that the thread-local physics space might be different from the RBC's physics space? I don't see how...

Taking this issue aside, it's still possible to add disabled controls to the physics space.
And while it may be bad practice/multithreadingly dangerous, it could very well be that there are multiple physics spaces, maybe even multiple per thread (if the implementation permits that). It could also be that the RBC is built on some scenegraph thread, but the physics space is in some physics thread. (regular case when starting the app state in parallel mode?)

Regardless of that, the API probably shouldn't care about such implementational implications, and even when taking that away, it's very odd to be able to set a physics space, when the implementation doesn't care and uses some "random other" physics space.

Regarding this issue, I'm now done with the low hanging fruit and will submit a PR shortly to have a look at them.
The remaining issues are:

• TestCloner
• TestCloneSpatial
• TestIDList

Because those are written in a "debug utility" manner, they may require more thought.
For instance the cloning could unit test cloning a list of jme standard classes (if not all, kinda like a fuzzy test) and then ensure equals() matches.
One could also imagine some reflection based comparisons on whatever comes to mind.
The current code requires someone who knows what he's looking for, so I couldn't really grasp what to test.

• TestRayCollision
  This may already be covered by com.jme3.collision.BoundingCollisionTest in a better fashion

• TestRayCasting

• TestHalfFloat
  Those require user input to be useful, so we may need to decide a few useful inputs.

• TestTempVars
  This is a benchmark, so there is nothing valuable to test. Asserting that one method is faster than the other would already fail on my machine and in general is heavily dependant on the circumstances without really making any statement.

[stephengold]

You're right. I was thinking of a different piece of code. The check in addCollisionObject() is redundant.

If a thread can have multiple physics spaces, then the per-thread data is meaningless. I think I resolved this paradox in Minie by giving every PhysicsCollisionObject a getCollisionSpace() method (and ignoring the per-thread data).