[#16] Multiple traces same time

[matanshukry]

The PR will continue and trace all objects found and not just the first.
Also included change to remove all traces on end, of course.

Solves #16

[rlewicki]

I think engine plugins are not allowed to use generic log categories. It would fail to compile and fail the submission to marketplace (fab). I'm happy to accept this PR if you can declare and define a log category for this inside MeleeTrace.h/cpp files? Something like LogMeleeTrace?

[matanshukry]

Looks like you already have, I just missed it.
Using it now, updated this PR.

[rlewicki]

This isn't really necessary. Each trace should have unique hash. Did you add this code just as a caution or did you actually run into a case where there were two traces active at the same time with identical hash?

[matanshukry]

@rlewicki I ran into the case.

In this PR I'm enabling adding multiple traces, which are added in InternalStartTrace - where we already have the hash. Hence all the traces added in there will have the same hash.

The issue without this part is that only the 1st trace would be removed; e.g. for 2 swords, the 2nd is left and kept tracing.

Probably the more "correct" code would be to make the hash unique by both the context object and the mesh object.
But honestly there's just no need; simply considering this hash is now a "group hash", and removing them all in this loop, works perfectly.

[rlewicki]

Hey @matanshukry! First of all, really sorry it took me so long to get to it and properly review it. I played around it and I'm afraid I cannot accept this pull request as is, because in my opinion it doesn't solve the #16. Let me explain why I think so, perhaps I am missing some important puzzle part here.

So, by removing the return statement in thet trace start, you allow to create more than a single ongoing trace instance on multiple meshes. Let's assume double sword for simplicity. If you use exactly same sword mesh for both hands with your method, starting trace using one sword, will also trigger it on a second one. Always. There is no way to make an attack sequence where only one sword is tracing at the moment. This is major design flaw as this basically limits you to only use symetrical double sword attacks? If there is another attack animation where you first slash with a left hand and then with the other, with your proposed changes it would always start tracing and end tracing on both.

So, with that being said, while I do agree that this is solving one issue, but at the same time it creates another one that make it impossible to create non-symetrical double weapon attacks. I'm not sure what would be a proper solution to this problem to be honest thus I think, for the time being, the best solution I can suggest is just maintaining two copies of skeletal meshes of your weapon and manage their tracing separately.

Let me know what's your take on that. Cheers!

[rlewicki]

Actually, I just had conversation with a friend and he suggested that we could instead allow user to filter which weapon to use based on a character socket to which the weapon is attached.

So, for example, if you only want to trace the left sword, you could specify weapon_l which is a socket defined on a character mesh which is sort of bound to the animation. I think this is the way.

[matanshukry]

@rlewicki you're sort of right, but the actual downside is just performance, not actual logic/issue.

There are 2 scenarios:

1. The animation includes movement for both swords - hence you'll want the trace to run for both. We're good here.
2. The animation includes movement for only 1 sword. The trace will run for both, but the non-moving sword is.. well, not moving. So the trace will simply trace on the non-moving sword. IMO that's fine; Unlikely enemies will actually walk towards a non-moving sword, but even if so - a sword is cutting even if someone walks towards it, and not only the other way around 🙃.

I will say that the method you're suggesting is better, but it doesn't seem like you have the time to make the PR, and my solution, while not optimal - works and ready. A bird in the hand is worth two in the bush :)

Either way feel free not to merge it, I'm using it already anyway and it's working great for me xd

[rlewicki]

I'm not sure I understand your second point.

The trace will run for both, but the non-moving sword is.. well, not moving. So the trace will simply trace on the non-moving sword.
This is definitely not the case for majority of the animations I have worked with. Let's take a look at this short video for reference.

https://youtu.be/TukuRVr3bgQ?si=7gV3mgUkR3TPrvBI

Pose 1:

Pose 2:

Clearly both swords, and the whole body of the character is moving but only one of the swords is attacking at the moment. You most certainly don't want both of them to be tracing here, and this is part of the same attack animation.

Regardless, I'm happy that your modification is working for you. I will keep this issue open until I have some spare time and implement the solution I proposed above.

Thanks for your contribution!

[matanshukry]

@rlewicki I'll comment on each pose:

Pose1: Not sure it's worth talking about, not even sure if you want the trace to be on entirely since it looks like a "hit" animation.
Pose 2: while the right sword is what you want to trace on, nothing bad will happen if you trace the left hand. It will only detect someone, if they come close enough to the sword - which honestly in games shouldn't really happen anyway. But imagine a real-life scenario; just because you're swinging the right sword, if someone else comes in contact with the left one - he will get stabbed.

But sure, I guess that's game specific. More options is always better.
Feel free to also close this if you want btw.