Pathfinder optimization

[SafwatHalaby]

Got rid of DoWithChunk, an order of magnitude less locks.

[madmaxoft]

I think the cPath constructor doesn't need the chunk either, only the Step function does, so how about losing the m_Chunk member variable, too, and propagating the chunk via parameters?

[SafwatHalaby]

But the cPath constructor performs the "initializer step" and checks the starting position and ending positions, it needs interactions with the world.

Regarding propagating via parameters: It's technically possible, but we'll needlessly pass that parameter everywhere, I find that a bit dirty, don't you think?
Also, if the old chunk must be discarded when GetNeighborChunk is called, it is impossible to use parameters only.

[SafwatHalaby]

regarding getting rid of the chunk in the constructor:
I could move the "initializer step" to the step function and then add:
if (ThisIsTheFirstStep) { PerformInitializerStep(); return; }
But that's an extra if, 20 times per sec per mob :P

[SafwatHalaby]

Awaiting comments / merge.
I'll rebase it when you want to merge.

[SafwatHalaby]

Is it legal to do it the other way around?: only passing the chunk via the constructor and using that chunk till the path calculation ends?

[SafwatHalaby]

This isn't exactly what you asked for, but I feel it's cleaner. Tell me what you think.

[madmaxoft]

No, that won't work. If the chunk gets unloaded between the constructor call and the path finish, this will fail horribly. You need to pass a valid chunk in each step.

I didn't see the initialization step in the constructor, that makes it necessary to pass the chunk there as well.

It isn't actually necessary to get rid of the "old" chunk when moving to a neighbor. The only time you have to consider your chunks invalid is when you exit out of the Tick function - at that moment every cChunk instance is in danger of being unloaded.

[SafwatHalaby]

Got it. Is it ok to keep the chunk as a member function?

[SafwatHalaby]

Rebased.

[madmaxoft]

I guess so. Perhaps add a comment to its declaration, say that it's valid only within the Step call.

[SafwatHalaby]

Awaiting comments / Rebase and merge.

[SafwatHalaby]

I'd like to fix the swimming / fence issue by editing Issolid, I'm going to include it in this PR because otherwise a merge conflict will occur. To avoid confusion I'll rebase and name the commit "Issolid rewritten" or something like that.

[NiLSPACE]

If you change the IsSolid function you might break other code that uses it.

[SafwatHalaby]

No, it's my PathFinder's Issolid, a different function.

[SafwatHalaby]

Fixed fence issue, swimming for later, awaiting comments / rebase and merge

[ghost]

I think for the fence, you need also check for E_BLOCK_FENCE_GATE.

[36451]

Nether brick, spruce, birch, jungle, dark_oak, acacia.
Fence gate comes in all those flavors too.

[ghost]

@Jan64 that doesn't matter, we don't need to check the data value of the fence. Only the block type itself.

[SafwatHalaby]

@Seadragon91 , you are right.
I observed that my fence fix isn't perfect, I'll do it thoroughly later in another pull request.

[SafwatHalaby]

Fencegate added.

[madmaxoft]

spaces

[36451]

@Seadragon91 Different kinds of fences are defined as different blocks, they don't only differ in data values.

[SafwatHalaby]

@Jan64 Looks like you're right, BlockID.h has lots of fences. I really don't want to mix pull requests so let's leave this at the current imperfect state, I'll need to take a different approach in dealing with fences anyways.

[SafwatHalaby]

Any non-fence related comments? I'm not pushing the issue aside, I just don't want to invest time on something I'm going to rewrite anyways.

[madmaxoft]

I personally would reset m_Chunk to nullptr at the end of the constructor and the Step function, and add ASSERT(m_Chunk != nullptr) to each function accessing the chunk, so that any mis-use is detected early.

[SafwatHalaby]

How about ASSERT(m_Chunk = nullptr || 1==1); or something like that? to get rid of setting the NULL needlessly in production?

[madmaxoft]

Not worth it. It took me a whole minute to understand what that assert is doing (that it's setting, not comparing, the m_Chunk), so it's not good for code clarity. Just put the assignment there, it's not worth micro-optimizing this much.

[SafwatHalaby]

Got some equavilant which is clearer?

[SafwatHalaby]

Perhaps there should be a Macro DoInDebugModeonly or something like that.

[SafwatHalaby]

Ok to squash?

[madmaxoft]

Fix the parentheses and squash.

[SafwatHalaby]

Squashed and fixed extra (.

[SafwatHalaby]

Note that my Github username has been changed from WiseOldMan95 to SafwatHalaby.