Flags and Bools #474
Flags and Bools #474
Conversation
|
I could be wrong on this - but I thought in the case of bools, the flag was the data. In all the other cases, the flag tells the client what to read. With bool flags, the flag is the data - and we get and set with bit arithmetic. Again, I could be way off base here. |
|
I'm not sure I follow what you're saying but let me share what my thinking on this change and see if makes sense... In PropertyBool I noticed several of the bools were either identical in naming or extremely close to the flags of ObjectDescription and PhysicsState. Given that, it leads me believe they were using the bools in some form to affect those two properties.. In the case of PhysicsState, using the bools, its easier to maintain changes from state to state and not have to worry about inadvertently blowing out a flag because the PhysicsState is changed by altering bools in WorldObject instead of using SetPhysicsState which meant you had to read in previous state, alter it (accounting for keeping other states) and send it back out. Examples of the improved way of handling them can be seen in TeleportInternal and HandleCloak. In both cases, I only change bools and don't have to worry about losing other flags set by other things. This is certainly identical, at least in the case of Cloaking, to how it acted in retail. If I turned on cloaking, I wouldn't reappear each time I teleported around the world, but instead would maintain the cloak. I'm not quite yet finished or happy with ObjectDescriptionFlag.. Based on the bools I've found, I suspect it was not as immutable as we are treating it currently.. As an example, I think its possible that in Player.cs, we would default the ObjectDescriptionFlag to be just I could see Player, Book, Vendor, PkSwitch, NpkSwitch, Door, Corpse, Lifestone, Food, Healer, Lockpick, Portal, and Bindstone all being "base" ObjectDescriptionFlags set in their respective classes in the initial creation. Those are all likely immutable.. Need to do more research of pcaps + some more better code work on my part than I roughed in yesterday to flesh that out |
|
I agree that flag can change - pk to non pk is a perfect example. The only thing I was saying is (and I am sure this is just a data saving measure) with all the other flags, you set it and that tells the client, hey - I am sending this down to you in this message. With bool flags like ObjectDescriptionFlag the flag itself is the data. The flag is sent, but no bools are actually sent to the client in the packet only the flag. If I set Attackable - I do not also send a true down to the client. We can either just make methods like you have to behind the scenes manipulate the flag and just update that one field or we can break each flag out as a bool as you are doing, store and update those, then build the flag from that data. It is really 6 of 1 1/2 dozen of the other. It was just something that tripped me up early on the bool flags are the data - I was looking for actual data in the packet stream and it is not sent. Just wanted to make sure we were all on the same page. |
There was a problem hiding this comment.
I'm unsure if I like this change or not. I believe this ultimately stems from my lack of understanding of its purpose.
Is it essential to keep these properties separate from their 32-bit flag field? It appears we have PropertiesBools for them, are these properties that are transmitted to the client?
Source/ACE.Entity/AceCharacter.cs
Outdated
| @@ -20,13 +20,21 @@ public AceCharacter(uint id) | |||
|
|
|||
| // Required default properties for character login | |||
| // FIXME(ddevec): Should we have constants for (some of) these things? | |||
| AceObjectDescriptionFlags = (uint)(ObjectDescriptionFlag.Stuck | ObjectDescriptionFlag.Player | ObjectDescriptionFlag.Attackable); | |||
|
|
|||
| AceObjectDescriptionFlags = (uint)(ObjectDescriptionFlag.Player | ObjectDescriptionFlag.Stuck | ObjectDescriptionFlag.Attackable); | |||
There was a problem hiding this comment.
These should probably set the field in AceCharacter, instead of call SetPropertyInt() (I'm not sure how I didn't notice this when you first made the change). Not sure if the change is appropriate as part of this PR, but it should be changed.
| { | ||
| WeenieHeaderFlag2 flags = SetWeenieHeaderFlag2(); | ||
| if (flags != WeenieHeaderFlag2.None) | ||
| IncludesSecondHeader = true; |
There was a problem hiding this comment.
The logic in these setters is getting slightly complex for my tastes (not necessarily a problem, if its essential), but its something you should think about. Don't want setting a property to have unintended/unexpected consequences for those unfamiliar with the code.
There was a problem hiding this comment.
PhysicsDescriptionFlag, WeenieHeaderFlag and WeenieHeaderFlag2 should really never be set manually by coding. They should always be set by the presence of data, further, WHF and WHF2 are sorta linked in that they both would be set at the same time AND if WHF2 is not None, it needs to add a flag to DescriptionFlag so the client knows to expect that data.
The changes I've made take it out of the hands of coding and puts it squarely on the data to determine what to do. As I see it, this is the best way forward with these fields.
Yes, I believe so. We need a proper way to allow for and track changes to these flags and in some cases these bools are sent as updates, either public or private, which are different from ObjectUpdates. There's a reason these bools are in the client like this imo. I'm adding a commit soonish to demonstrate the work which proves this out. |
|
Added MRT command to prove out changes. In order to make it work, do the following:
|
Setting up more bools intended to set flags for ObjectDescriptionFlag and PhysicsState