Unity-Technologies / EntityComponentSystemSamples

Other
7.25k stars 1.62k forks source link

Tanks Tutorial Inaccuracies #286

Open samueldcorbin opened 8 months ago

samueldcorbin commented 8 months ago

The readme here has many incorrect statements that do not match the code: https://github.com/Unity-Technologies/EntityComponentSystemSamples/blob/master/EntitiesSamples/Assets/Tutorials/Tanks/README.md

This isn't in the readme, but Step 1 links https://github.com/Unity-Technologies/EntityComponentSystemSamples/blob/master/EntitiesSamples/Assets/Tutorials/Tanks/Step%202/TurretRotationSystem.cs which says:

    // Unmanaged systems based on ISystem can be Burst compiled, but this is not yet the default,
    // so we have to explicitly opt into Burst compilation with the [BurstCompile] attribute.
    // It has to be added on BOTH the struct AND the OnCreate/OnDestroy/OnUpdate functions to be
    // effective.

This stresses that the attribute must be on both the struct and the method, but the code doesn't put it on the struct. I believe the comment is the part that is wrong, but I'm not sure.

Step 3 in the readme says: Introduces scheduling a parallel job. but the code in those files does not schedule any job at all.

Step 4 says Introduces aspects, entity prefabs, EntityCommandBuffer, and IJobEntity. but the code does not actually include an IJobEntity or an EntityCommandBuffer (although it probably should since it's performing a structural change!)

The file in step 4 also has an inconsistent comment here: https://github.com/Unity-Technologies/EntityComponentSystemSamples/blob/master/EntitiesSamples/Assets/Tutorials/Tanks/Step%204/TurretAspect.cs#L18 (it talks about making it Optional, but doesn't actually do anything to make it Optional).

Step 5 has several issues. The link to CannonBallAspect.cs is broken. The description says it uses the Aspect, but it actually just uses the Cannonball component directly (despite creating the Aspect in the previous step). The description says Introduces scheduling a parallel job with IJobEntity. but it doesn't actually use ScheduleParallel (it just uses Schedule). Also Step 3 was already supposed to introduce scheduling a parallel job (though it doesn't actually do that).

Step 6 has a mysterious unexplained state.RequireForUpdate<Execute.TankSpawning>(); here that I've never seen before: https://github.com/Unity-Technologies/EntityComponentSystemSamples/blob/master/EntitiesSamples/Assets/Tutorials/Tanks/Step%206/TankSpawningSystem.cs#L16

Step 7 says Introduces enableable components (IEnableableComponent), live conversion, and storing basic configuration data in a singleton. but the config singleton was created in the previous step. The linked file also has state.RequireForUpdate<Execute.SafeZone>(); again, with no explanation again. What is Execute.SafeZone and how is it different from SafeZone?!

Step 8 again has the mysterious state.RequireForUpdate<Execute.Camera>(); in https://github.com/Unity-Technologies/EntityComponentSystemSamples/blob/master/EntitiesSamples/Assets/Tutorials/Tanks/Step%208/CameraSystem.cs which is even more mysterious here because there isn't anything in the file even called Execute.

ConanGentleman commented 3 months ago

I agree with you