Review Request / Merge : Move to Object, Move to Position, Turn to Object, Turn to Position #328

ogmage78 · 2017-05-05T01:39:16Z

This is 99% of the way there. I have a bug I am having trouble tracking down. I think this is solid code once we hit that last bug. It does not break anything and we have a lot of people working on items related to this. The sooner this is merged - the better. In the absence of finding the bug, I would propose merging it anyway to get the core in.

To see the bug. @ci 12748 will create a training wand in your back pack. Drop it, move off - select it. It should put your pick up item on hold, move you to the position, then once in range pick up the wand. I was moving but now it is not. It has to be something simple, it was working before. Please advise.

One more thing - there are 4 files that show changes that I did nothing to - they show complete file replacements. I did not even edit them. Not sure why that is happening.

SetModel.cs, Extensions.cs - the solutions files - no idea why they show complete replace and no idea how to stop it or fix it.

…verflow

…ate Machine added - one bug left.

LtRipley36706 · 2017-05-05T16:36:09Z

Source/ACE/Network/GameAction/Actions/GameActionAutonomousPosition.cs

+                session.Player.BlockedGameAction = null;
+                session.Player.MoveToPosition = null;
+                session.Player.ArrivedRadiusSquared = 0.00f;
+            }
        }
    }
 }


end file on new line please

LtRipley36706 · 2017-05-05T17:06:30Z

Source/ACE/StateMachines/Enum/ContainerStates.cs

+        Unlocked,
+        InUse,
+    }
+}


newline eof

ogmage78 · 2017-05-05T17:06:43Z

@LtRipley36706 fixed. BTW did you do the move ci & create to debugObject? I think when you did the full rest it brought that code in. There is a bug - it will not spawn 12748 (training wand) in the backpack using ci. It will float it mid air using create, but it thinks it is a PK alter. I started looking at it - but while I think the way you implemented it is awesome - I am not sure I fully understand it. I am trying to work through the code now. If / when you had a minute to take a look - that would be awesome.

LtRipley36706 · 2017-05-05T17:06:47Z

Source/ACE/StateMachines/Enum/MovementStates.cs

+        Arrived,
+        Abandoned,
+    }
+}


newline eof

done. Sorry - I am aware of that now and will make sure I am doing it correctly going forward.

LtRipley36706 · 2017-05-05T17:07:28Z

changelog.md

+* State Machine
+* Currently one last bug to be addressed but wanted to get this in.
+
+### 2017-05-04


remove duplicated date

…t is correct.

…s :) Sorry about that. I will remember going forward.

LtRipley36706 · 2017-05-05T17:39:51Z

@ogmage78 yeah I swapped ci/create DebugObject to get the debug data for assess. The problem is DebugObject is trying to be this generic catch all and I'm not even sure it can be, adding in too much stuff alters how an object was really setup in retail. As of this moment, I'm still of the belief that essentially we're needing a 1:1 recreation of each weenie in the codebase so in order to properly spawn a Training Wand, you need a TrainingWand.cs weenie file with all the proper settings in there, and then we need a "factory" that searches and finds the right weenie to create an object from, assuming a weenie code file has been created for the object.

ogmage78 · 2017-05-05T17:51:25Z

@LtRipley36706 @Mogwai-AC Not dead set against that - but maybe it is just the database guy in me - see a db approach as a better choice. It seems what we need i(f I understand a weenie class correctly) the weenie class is the base template from which 0-N variations are all derived. Some like the training wand - is really the base and that is it. If we made weenie_class have all of the base data then have a 1 to N relationship to base ace object that would do the trick. Let's look at the use case of creating loot. Let's say we want to spawn armor for a corpse loot drop. Random pick the weenie class from the type armor. Pull up that base weenie class. Now do your mutations. Now - you can do the same thing with a file / class for every weenie class - I am not really sure if one way is better than the other. If we go db route, we could do one per type - armor, weapons, clothes etc many fewer files and going forward - a way to have content creators be able to create new things without code changes. Let me know what you think. I added Mogwai just to get his opinion. I don't feel strongly either way - and can work with whatever we decide.

Lidefeath · 2017-05-05T18:34:12Z

I am not yet convinced why we can't create most if not all base items / objects only based on their weenie from the database. When it comes to variations we will need some coding for it, which may be sensible to have in their own classes: I.e. to create the random variance in weapon damage is another formula than to create the random AL some armor items got.

Mogwai-TheFurry · 2017-05-05T18:45:31Z

When it comes to monsters, the weenie is essentially all that matters. The debate is whether or not it applies to NPCs, portals, etc. Items start from the weenies, but the buffs and stats are what vary.

Mogwai-TheFurry

In summary, I'm ok with the heart of the changes. I want that state machine to be named "movementStateMachine" (because we will have others) and to be private in the player. The player controls the state machine, not all the network handlers.

Mogwai-TheFurry · 2017-05-07T01:10:54Z

Source/ACE/Network/GameAction/Actions/GameActionAutonomousPosition.cs

+            if (session.Player.Statemachine.CurrentState != (int)MovementStates.Moving) return;
+            if ((Math.Abs(session.Player.PhysicsData.Position.SquaredDistanceTo(session.Player.MoveToPosition)) <= session.Player.ArrivedRadiusSquared))
+            {
+                session.Player.Statemachine.ChangeState((int)MovementStates.Arrived);


I don't like hitting the StateMachine of the Player from here. Perhaps we have a session.Player.OnArrived() method?

We'd also have a session.Player.IsPerformingMoveAction that his the state machine.

edit: even better: session.Player.OnAutonomousMove() that does it all. It would also do the call to UpdatePosition, I think.

Mogwai-TheFurry · 2017-05-07T01:11:10Z

Source/ACE/Entity/Creature.cs

+        /// <summary>
+        /// Track state of creature if their current action is blocked.   Either until they are no longer blocked or the action is abandoned.
+        /// </summary>
+        public StateMachine Statemachine = new StateMachine();


I'd really rather this be private.

Mogwai-TheFurry · 2017-05-07T01:12:41Z

Source/ACE/Network/GameAction/Actions/GameActionMoveToState.cs

@@ -92,6 +92,15 @@ public static void Handle(ClientMessage message, Session session)
            forcePositionTimestamp = message.Payload.ReadUInt16();
            message.Payload.ReadByte();

+            if (session.Player.Statemachine.CurrentState == (int)MovementStates.Moving)


as above, session.Player.OnActionMoveToState(ushort timestamp) or something.

Mogwai-TheFurry · 2017-05-07T01:14:05Z

Source/ACE/Network/GameMessages/Messages/GameMessageAutonomousPosition.cs

@@ -18,6 +19,8 @@ public GameMessageAutonomousPosition(WorldObject worldObject)
                Writer.Write(worldObject.Sequences.GetCurrentSequence(SequenceType.ObjectTeleport)); // teleport_timestamp - always 0 in my pcaps
                Writer.Write(worldObject.Sequences.GetCurrentSequence(SequenceType.ObjectForcePosition)); // force_position_timestamp - always 0 in my pcaps
                Writer.Write(1u); // contact - always "true" / 1 in my pcaps
+                if (p.Statemachine.CurrentState == (int)MovementStates.Idle)
+                    p.Statemachine.ChangeState((int)MovementStates.Moving);
            }


again, if we make StateMachine private, you force this logic into the player class where it belongs.

Mogwai-TheFurry · 2017-05-07T01:15:24Z

Source/ACE/Network/Motion/UniversalMotion.cs

+                            writer.Write(WalkRunThreshold);
+                            writer.Write(heading);
+                            writer.Write(RunRate);
+                            // TODO: This needs to be calculated and the flag needs to be deciphered Og II


yay properties instead of magic values 👍

…age on pickup.

…Please review for the changes you requested.

Mogwai-TheFurry

Good work on the changes to the state machine. You resurrected a bad method though.

Mogwai-TheFurry · 2017-05-08T11:47:02Z

Source/ACE/Entity/Landblock.cs

+        /// </summary>
+        /// <param name="objectGuid"></param>
+        /// <returns></returns>
+        public WorldObject GetWorldObjectByGuid(ObjectGuid objectGuid)


Mogwai-TheFurry · 2017-05-08T11:48:15Z

Source/ACE/Network/GameMessages/Messages/GameMessageAutonomousPosition.cs

-                Writer.Write(worldObject.Sequences.GetCurrentSequence(SequenceType.ObjectForcePosition)); // force_position_timestamp - always 0 in my pcaps
-                Writer.Write(1u); // contact - always "true" / 1 in my pcaps
-            }
+            if (!(worldObject is Player)) return;


skipping the { } is ok, putting return on the same line is not

I will get this - I really will :)

Still needs fixing.

Mogwai-TheFurry · 2017-05-08T11:49:59Z

Source/ACE/Entity/Landblock.cs

+            var arrivedRadius = (float)Math.Pow((targetWorldObject.GameData.UseRadius + csetup.Radius + 1.5), 2);
+
+            // are we close enough?   If so, our work here is done.
+            if ((player.PhysicsData.Position.SquaredDistanceTo(targetWorldObject.PhysicsData.Position) <= arrivedRadius)) return true;


return on it's own line, please.

Mogwai-TheFurry · 2017-05-08T11:50:38Z

Source/ACE/Entity/Landblock.cs

+            player.MoveToPosition = targetWorldObject.PhysicsData.Position;
+            player.ArrivedRadiusSquared = arrivedRadius;
+            player.BlockedGameAction = action;
+            player.OnAutonomousMove(targetWorldObject, movementType);


So, checking for "InRangeForAction" triggers an OnAutonomousMove? This defies logic to me.

yea now that you point it out - a combined two actions that should each be their own method. I think I have to do all the math to see if I am in range. I am setting a destination, a range and saving off the original action (pick something up, use something etc) and then starting on the way. I guess the best way to do it is to have InRangeForAction return arrivedRadiusSquared - if non 0 then I can take the appropriate action. So let it be written, so let it be done.

…tate

ogmage78 · 2017-05-09T03:52:48Z

I think all the bugs are fixed and all of the changes are complete.

Mogwai-TheFurry · 2017-05-09T13:00:12Z

Source/ACE/Entity/DebugObject.cs

@@ -77,6 +77,7 @@ public DebugObject(ObjectGuid guid,  BaseAceObject baseAceObject)
                // Creating from a pcap of the weenie - this will be set by the loot generation factory. Og II
                this.PhysicsData.PhysicsDescriptionFlag &= ~PhysicsDescriptionFlag.Parent;
                this.GameData.ValidLocations = (EquipMask)baseAceObject.ValidLocations;
+                this.GameData.IconOverlay = (ushort)baseAceObject.IconOverlayId;


Icons are packed dwords in most places. I doubt this cast is valid.

GameData.IconOverlay is defined as a ushort. The iconOverlayId is defined in BaseAceObject as an int. It is an int in the data as well. so do I take out the 0x06000000 from the number in the database and just store last 4 up to FFFF which will fit in a ushort or change or change gameData? I am assuming strip the number - I need to make sure the writer is adding the 0x6000000 back to to iconoverlay and underlay first. Let me know and I will fix it.

If I remember correctly we went the way of changing the type to int in the other cases were this was a problem as well, i.e. textures, palettes, etc.

Mogwai-TheFurry · 2017-05-09T13:01:09Z

Source/ACE/Entity/Landblock.cs

@@ -276,7 +276,7 @@ public List<WorldObject> GetWorldObjectsInRange(WorldObject wo, float maxrange,
            return allworldobj;
        }

-        public List<WorldObject> GetWorldObjectsByGuid(ObjectGuid objectguid, bool neighbors)
+        private List<WorldObject> GetWorldObjectsByGuid(ObjectGuid objectguid, bool neighbors)
        {
            List<WorldObject> allworldobj = new List<WorldObject>();
            lock (objectCacheLocker)


Copying the list, then looking for 1 object is a hell of a lot slower than just looking for the 1 while you have the lock.

Mogwai-TheFurry · 2017-05-09T13:03:47Z

Source/ACE/Entity/Landblock.cs

@@ -276,7 +276,7 @@ public List<WorldObject> GetWorldObjectsInRange(WorldObject wo, float maxrange,
            return allworldobj;
        }

-        public List<WorldObject> GetWorldObjectsByGuid(ObjectGuid objectguid, bool neighbors)
+        private List<WorldObject> GetWorldObjectsByGuid(ObjectGuid objectguid, bool neighbors)
        {


Guid is unique. Or it is designed to be so, anyway. Duplicate guids = big bug. Returning a List here is a waste.

Locking just to copy the entire list is a waste. Check for the item in the lock.

If you find it here, there's no reason to hit the adjacencies.

If you find it in an adjacency, either return or continue out of the loop to stop looking.

I missed that I could use the other call and search adjcency - this is something stackoverflow put it - because I did not realize that the other could do what I wanted - I used this to search the landblock plus adj. I thought it was funky but I was concentrating on finding my damn bug and did not really think about it.

Mogwai-TheFurry · 2017-05-09T13:04:10Z

Source/ACE/Entity/DebugObject.cs

@@ -77,6 +77,7 @@ public DebugObject(ObjectGuid guid,  BaseAceObject baseAceObject)
                // Creating from a pcap of the weenie - this will be set by the loot generation factory. Og II
                this.PhysicsData.PhysicsDescriptionFlag &= ~PhysicsDescriptionFlag.Parent;
                this.GameData.ValidLocations = (EquipMask)baseAceObject.ValidLocations;
+                this.GameData.IconOverlay = (ushort)baseAceObject.IconOverlayId;


Elsewhere, Icon is a Packed DWORD. I doubt this cast is valid.

Mogwai-TheFurry · 2017-05-09T13:04:49Z

Source/ACE/Entity/Landblock.cs

+        /// <returns></returns>
+        private WorldObject GetWorldObjectByGuid(ObjectGuid objectGuid)
+        {
+            var objectList = GetWorldObjectsByGuid(objectGuid, true);


See, this is the method you needed, but with the adjacency flag. Combine this with the bad method above.

Mogwai-TheFurry · 2017-05-09T13:06:14Z

Source/ACE/Network/GameMessages/Messages/GameMessageAutonomousPosition.cs

-                Writer.Write(worldObject.Sequences.GetCurrentSequence(SequenceType.ObjectForcePosition)); // force_position_timestamp - always 0 in my pcaps
-                Writer.Write(1u); // contact - always "true" / 1 in my pcaps
-            }
+            if (!(worldObject is Player)) return;


Still needs fixing.

Mogwai-TheFurry · 2017-05-09T13:07:01Z

Source/ACE/Network/Motion/UniversalMotion.cs

@@ -9,6 +9,16 @@ namespace ACE.Network.Motion
    public class UniversalMotion : MotionState
    {
        public uint Flag { get; set; } = 0x0041EE0F;


whitespace lines between members

Mogwai-TheFurry · 2017-05-09T13:07:43Z

Source/ACE/StateMachines/Rules/ContainerRules.cs

+
+        private static int initstate = (int)ContainerStates.Unlocked;
+
+        private static readonly List<Rule> Rulesstates = new List<Rule>();


private members = camel case (rulesStates)

Mogwai-TheFurry · 2017-05-09T13:07:55Z

Source/ACE/StateMachines/Rules/ContainerRules.cs

+    {
+        private static bool init = false;
+
+        private static int initstate = (int)ContainerStates.Unlocked;


camel case (initState)

Mogwai-TheFurry · 2017-05-09T13:08:16Z

Source/ACE/StateMachines/StateMachine.cs

+{
+    public class StateMachine
+    {
+        private List<Rule> validrules;


camel case (validRules) and whitespace lines

…nd replace. Fixed bug in state machine, addressed Mogwai's issues.

ogmage78 · 2017-05-10T00:01:40Z

Ok - I think this is ready for another review.

Major rip and replace to stop reaching into landblock and passing out World Objects. I tested all of my stuff and as many items as I could think of and they all appear to work. May want to have others test their stuff. I think this is worth merging even if it did break stuff, as it is much cleaner and less confusing I think. Also, any issues discovered with be isolated to movement updates and localized to sequence.
Fixed bug from 05/04 Og II update.

ogmage78 changed the title ~~Review Request: Move to Object, Move to Position, Turn to Object, Turn to Position~~ Review Request / Merge : Move to Object, Move to Position, Turn to Object, Turn to Position May 5, 2017

ogmage78 added 5 commits May 4, 2017 20:54

Testing fix

9ed489e

Inital foundation for motion - again. Up to state machine from Stacko…

1acf02e

…verflow

Motion MoveToObject, MoveToPosition, TurnToObject, TurnToPosition. St…

b42b962

…ate Machine added - one bug left.

Updated Change Log

e0c199c

did a rebase - first time that ever worked.

cc6682a

ogmage78 force-pushed the movestate branch from 57a0735 to cc6682a Compare May 5, 2017 01:57

ogmage78 and others added 5 commits May 4, 2017 23:43

Continued to debug. Getting closer.

b556ea4

Finally found the any cpu setting. Man that was obscure....

e2c8df0

Merge branch 'master' into movestate

5a5836f

Merge

8ec2c57

Merge

4f144ce

LtRipley36706 reviewed May 5, 2017

View reviewed changes

Fixed EOF position.

0f373b5

LtRipley36706 reviewed May 5, 2017

View reviewed changes

ogmage78 added 3 commits May 5, 2017 12:07

Fixed another EOL - I got this now - I will be looking to make sure i…

e0023a9

…t is correct.

Fixed error in change log

0cb616a

I will never do another file with the EOL not on a new line 1000 time…

5c9f9cb

…s :) Sorry about that. I will remember going forward.

Mogwai-TheFurry suggested changes May 7, 2017

View reviewed changes

ogmage78 added 3 commits May 6, 2017 23:10

post merge update

5be75f3

WIP - this is looking better - still have a bug sending a delete mess…

441a133

…age on pickup.

Made changes for Mogwai - still chasing a bug on subsequent pickups. …

882d87e

…Please review for the changes you requested.

Mogwai-TheFurry suggested changes May 8, 2017

View reviewed changes

ogmage78 and others added 3 commits May 8, 2017 09:00

Merge branch 'master' of https://github.com/ACEmulator/ACE into moves…

875ab8f

…tate

WIP

61f0534

Finally got this working. Good to go.

dd854ba

Update Creature.cs

a90c4eb

Mogwai-TheFurry suggested changes May 9, 2017

View reviewed changes

Removed all use of passing worldobject out of landblock - major rip a…

44f6e14

…nd replace. Fixed bug in state machine, addressed Mogwai's issues.

Mogwai-TheFurry added 4 commits May 10, 2017 07:42

Update BaseAceObject.cs

da2fea1

Update StateMachine.cs

b07cb2c

Update Landblock.cs

20dd137

Update Landblock.cs

a97c9a8

Mogwai-TheFurry merged commit fe38f2f into ACEmulator:master May 10, 2017


		private static int initstate = (int)ContainerStates.Unlocked;

		private static readonly List<Rule> Rulesstates = new List<Rule>();

Review Request / Merge : Move to Object, Move to Position, Turn to Object, Turn to Position #328

Review Request / Merge : Move to Object, Move to Position, Turn to Object, Turn to Position #328

Conversation

ogmage78 commented May 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogmage78 commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LtRipley36706 commented May 5, 2017 • edited

ogmage78 commented May 5, 2017 • edited

Lidefeath commented May 5, 2017

Mogwai-TheFurry commented May 5, 2017

Mogwai-TheFurry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mogwai-TheFurry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogmage78 commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogmage78 commented May 10, 2017

ogmage78 commented May 5, 2017 •

edited

LtRipley36706 commented May 5, 2017 •

edited

ogmage78 commented May 5, 2017 •

edited