Job class is unnecessarily hard to access

[jankrassnigg]

I've finally written the code to use our existing job system for Jolt, but ran into a couple of unnecessary roadblocks.

The Job class is defined inside JobSystem. It is also in a protected area. That means only a class that derives from JobSystem is able to create a custom Job class (to derive from it). It also means that storing pointers to JobSystem::Job in any other piece of code is difficult, because the declaration is inaccessible.

I have a custom Task class. This had to somehow reference a Jolt Job. So now it holds a void* to a Job. But then it also can't call "Execute" and "Release" on the Job. Therefore I have to forward this to a (static) function on my custom JobSystem implementation, which then just casts the void* to a Job* to call those functions.

It would be much easier, if the Job class was not a (protected) class inside JobSystem, but simply a free-standing class.

Additionally, functions like void QueueJob(Job* pJob) just give you a Job pointer. But I have to start my own task here, and I don't want to do expensive lookups to get from Job* to my own thing. That's why I derived from Job, so that I could store an int in the Job, as an index. I could imagine, that this is a common problem, you need to somehow get from JPH::Job to your own thing. So it might be useful to simply add a void* userData directly to JPH::Job.

[jrouwe]

The Job class is defined inside JobSystem. It is also in a protected area. That means only a class that derives from JobSystem is able to create a custom Job class (to derive from it). It also means that storing pointers to JobSystem::Job in any other piece of code is difficult, because the declaration is inaccessible.

This was actually on purpose because nobody should be using the Job class besides the JobSystem and its inherited classes. All other classes should be using JobHandle instead.

I have a custom Task class. This had to somehow reference a Jolt Job. So now it holds a void* to a Job. But then it also can't call "Execute" and "Release" on the Job. Therefore I have to forward this to a (static) function on my custom JobSystem implementation, which then just casts the void* to a Job* to call those functions.

Looking through your change:

ezEngine/ezEngine@fdde4af

Wouldn't a simple solution be to make ezJoltTask a protected class inside ezJoltJobSystem? I think this allows you to fix both of the mentioned issues.

That's why I derived from Job, so that I could store an int in the Job, as an index. I could imagine, that this is a common problem, you need to somehow get from JPH::Job to your own thing. So it might be useful to simply add a void* userData directly to JPH::Job.

I am not sure if your solution is safe. Inside ezJoltJobSystem::Execute you Release the Job which will immediately make it available for reuse. This means that before your ezJoltTask::Execute function returns, the Job could already be reused and the ezJoltTask could be rescheduled before it finishes. If allocating tasks is expensive, I suggest keeping a separate freelist and handing them out as they become available. As jobs and tasks cannot atomically finish at the same time, I don't think it is possible to have a 1-on-1 relation between them.

[jankrassnigg]

I've tried this, and I was indeed able to fix my problem that way.

[jankrassnigg]

Wouldn't a simple solution be to make ezJoltTask a protected class inside ezJoltJobSystem? I think this allows you to fix both of the mentioned issues.

I can give that a try. I'm generally not a fan of declaring classes inside classes, because that makes it impossible in C++ to forward declare types. Though even if I can work around it this way, it may still be an issue for other people with yet different use cases.

This was actually on purpose because nobody should be using the Job class besides the JobSystem and its inherited classes. All other classes should be using JobHandle instead.

Hmm, I guess that's possible, but that wasn't obvious to me, seeing that the JobSystem interface takes Job pointers, not handles.

I am not sure if your solution is safe.

Haha, yes, you are correct. I already have a fix on a branch because I've run into that race condition a little after I completed the PR.

[jrouwe]

Hmm, I guess that's possible, but that wasn't obvious to me, seeing that the JobSystem interface takes Job pointers, not handles.

All of the interfaces that takes a Job pointer are protected, and they're all for the internal implementation.