refactor: Rewrite AMF and property behavior logic to use smart pointers, references, and string_views over raw pointers and std::string&

[jadebenn]

Not sure what's causing the game tests to fail right now after several hours of debugging, so marking it as draft for now. If any of you have a bit of time to look it over, it would be greatly appreciated.

[Jettford]

Way, way too many unrelated changes, this should be broken up.

[jadebenn]

Ah. Sorry about the commit history. I pulled the TryParse branch over before it was added and forgot to squash it, so all those commits were added to this branch's history.

[jadebenn]

Way, way too many unrelated changes, this should be broken up.

I actually stayed really focused. The commit history is just screwed.

[Jettford]

I am not basing my comment off of the commit history, I didn't read that.

[jadebenn]

Well, for what it's worth, I at least fixed the commit history now.

[EmosewaMC]

There is a lot here to unpack so ill bullet the points so they are clear, and also to note that i only got half way for now just so there werent too many questions to answer at once

1. Why is this code being changed?
   a. This code is well tested (even unit tested unlike much of the code base) and has
   been proven to work and be good enough for our use case.
2. How much performance are we really saving with a custom string hasher on small
   strings?
   a. AMF3 is not something used in a large portion of the codebase. Alongside that,
   the strings used in it are generally small enough to fit in SSO. Also,
   there are generally so few keys per AMF associative that the lookup is already
   only 1 comparison so this seems like we're just adding complexity with very little gain.
3. Why are we using unique_ptr for the AMF values when they are just being side cast
   away all the time with .get and .reset?
   a. The whole purpose of unique_ptr is the object is owned and destroyed at the end
   of scope. Seeing so many uses of .get, .reset and with all of the moves going on; It is fairly mis-leading what
   actually owns the object at the end of scope and appears to defeat the purpose of the class.
4. Why are we changing the functions in the AMFDeserialize header to be static?
   a. This just seems like an unnecessary change. If they are private static, they may
   as well only live in the cpp file anyways without a need to be declared in the class.
5. Why are we using complex emplace in the insertion for the associative
   part?
   a. This is not a feature many people know how to use, as such this is something
   others would look at and just remove and replace with a regular emplace as this would cover our use case.
6. Why are we using an incredibly complex string_view hasher for what will be very little
   performance gain and a very difficult to maintain struct?
   a. Same as point 2; What are we gaining from this? This also uses very complex
   folding expressions that are hard to understand the syntax of
   for those who are not fluent in meta programming. Yes you did explain the end
   result and an example use case of the structure, but the way the overload struct is written implies it is used for more than
   just string hashing, and more so a way to overload any lookup with dual types.
7. Why are we suddenly using rvalue references for container iteration?
   a. The change here is a bit syntactical and is another one that is hard to understand
   and maintain for others.
8. If we are updating all ControlBehaviorMessages to have m_ prefixes, why was
   BlockDefinition skipped?
9. Why are variable declaration orders being shifted around?
   a. Is there a particular end goal with the end format you landed on?

MergeStripsMessage.h. I stopped around this file for a reference where to begin again
after these questions are answered

[jadebenn]

Hey Emo: Thanks for looking over this! I'll try and address your questions point by point.

1. It's just a proposal as any PR is. However, I am trying to cut down on manual memory management wherever possible in the codebase, and the legwork required to do so did turn out to be more than I anticipated. My intention here is to make the ownership of resources more clear.
2. It's my understanding that a temporary std::string is constructed by default as part of the lookup process due to C++ legacy reasons, so it's nice to be rid of that. The main reason, though, was to allow more widespread usage of std::string_view, for reasons which I'll expand on in the next few points.
3. get() is being used for non-owning references. The dense and associative vectors/maps own the actual resources, which is why they're wrapped in unique pointers to begin with. If polymorphism wasn't a concern (and it unfortunately is), I would have just stored the objects in the vector directly. As-is, when those containers go out of scope, the unique pointers ensure their destructors are called as well. Reset() is being used when an element needs to explicitly be deleted or replaced - almost 1:1 with the calls to 'delete' in the unaltered code.
4. I'm fine with moving those static functions to the cpp file. I just noticed that a few of those functions did not actually work on 'this' objects and could be made static with no issue.
5. Not sure what you mean by a 'regular emplace,' here.
6. See point 2. To elaborate on my earlier point (and point 3 as well), the use of raw pointers in the original code made it pretty unclear when a resource was owned and when it was being provided as a reference. This is also the case for strings. Now, std::string is only used in contexts where the actual string's ownership needs to be passed around, just as unique ptr is only being used to convey the deletion responsibility is being shifted to someone else.
7. This can be replaced by a regular auto& in this context and I will be more than happy to if you still think it should be after I explain my reasoning. Both const auto& and auto&& are 'universal references' and will bind to anything. Given that, I prefer to use const auto& to signify a 'read only' loop and auto&& to signify a 'read-and-write' loop. In this particular case an auto& will do that job just as well, but an auto&& reference guarantees it in any context.
8. Oversight.
9. Smaller variable types (ints, floats, etc) listed first to (potentially) reduce padding. Not really a real-world concern here, but good practice and I didn't see the harm in doing it.

[jadebenn]

I will also note that, to my understanding, the only issue blocking the CIs currently is some strangeness with the unit tests themselves. While in game, the actual behaviors seem to load fine (at least according to the debug console output), in the game tests their IDs do not. I spent several hours trying to figure out the cause of this and I wasn't able to. Part of what motivated me to post this as a draft was the hope that one of you might have some insight.

[jadebenn]

assuming the CI issue was resolved in these last few commits, I'm going to pivot to splitting up this big PR into smaller ones,

[EmosewaMC]

the only 2 things i have time to reply to right now are

Smaller variable types (ints, floats, etc) listed first to (potentially) reduce padding. Not really a real-world concern here, but good practice and I didn't see the harm in doing it.

Unless the ordering is causing an actual problem, it can be left alone to reduce changelist size. This is not an optimization we are going for and will have next to zero effect for our use case, though i appreciate the consideration and thought behind it. Feel free to leave it in this PR and the other one, but in the future, we would rather focus on higher order optimizations than micro ones like packing outside of binary stream data.

It's just a proposal as any PR is. However, I am trying to cut down on manual memory management wherever possible in the codebase, and the legwork required to do so did turn out to be more than I anticipated. My intention here is to make the ownership of resources more clear.

We would rather, as mentioned in discord, propose the changes before the PR is put up. If a PR is put up first, then the work has already been done and we would rather not throw away work if we dont need to. In this context manual memory management was a necessary cost for abstraction and with the previous implementaion already finished and working, this is just change for the sake of change. It's not that i'm against the changes themselves, but more the code for AMF was finished as far as functionality goes and changing it may introduce bugs from a new implementation. Removing manual memory management is a good thing to work on, but in a context like this I do not see it as necessary.

[jadebenn]

Reviving this PR since some of the changes have been incorporated through other pushes and it shouldn't be as overwhelming anymore.

[EmosewaMC]

lgtm

stale