AutoMove and Sneak

[lgromanowski]

I bound up Auto move and Sneak and I began outlining AutoMove.

I need to think of a place to place these booleans to check if the player is sneaking or Automoving - I'd figure it'd be in Player, but I'd figure I'd ask if there is a better place to store this information.

Just need to be able to get the players current direction and apply the current Movementstate's speed. However, I'm not sure if the XYZ values as show in the inputmanager.cpp are relative to the player or relative to the world.

Maybe you should drop sneak for now. There is no visual indication for sneaking yet (which will make testing harder) and there are possibly also gameplay-implementation aspects to consider.

For the auto-move: I make you a list. Really, its easier that way. I know the whole architecture very well and can tell you exactly what to place where. That has to wait until tomorrow though. Don't have time for it right now (still busy with the tab-completion stuff).

btw. your last commit reverted the submodule settings. Looks like you forgot to do a submodule update.

Keeping Sneak active requires use of NPC visibility cones - I'll leave that alone until I know more about it and the NPCs are all finished.

Thanks for the list - I know I could stare at it for a while, but it's just going to take a while of finding things to fill in in order to get intimate with the whole thing. The reality is that I didn't code it myself and it has no comments. I'll be adding notes for everyone, though.

Looks like the player axis is local to the player. That makes this a lot easier - I don't have my discrete math and linear algebra yet.

All Ill have to do is just either call the bind forward (force hit the button) for the player or just apply speed to the Z Axis instead.

Just don't know what class is storing the player's movement information.

I did "git submodule update" - it resolved but no output.

I did "git submodule update" - it resolved but no output.

Well, your previous commit reverted the submodule changes, so update doesn't have any effect. Don't worry, I'll fix it in the merge. Just make sure it doesn't happen again.

Here is your list:

1. Add a bool variable (mAutoRun or something) to World::Player, including the usual get and set functions.

2. In InputManager bind the auto run action (don't forget to assign a key!) to a InputManager function (toggleAutoRun or something), that toggles the auto run state in Player via the get and set functions you made in 1.

3. For each movement key pressed, disable auto run via the Player set function.

4. Now add a function pulse (float duration) to Player (duration is the time since the last call in seconds). Check if auto run is set, call the movRel function of Player. You want X=0 and Y=0. Set Z to a suitable factor multiplied by the duration multiplied by -1. You will have to experiment a bit here, but don't spend too much time on finding the right factor. It will be replaced, once we have more game mechanics in place.

5. Finally call the pulse function from Engine::frameStarted. You can get the duration value from the function argument. Here is the API: http://www.ogre3d.org/docs/api/html/str ... Event.html

I started - also, I'm going to see if I can draw up some nice "every session" instructions for the wiki, since I'm noticing that doing something out of order can make problems with one's git fork.

Man - if only all this stuff were automatic.

Basically, if you jump into coding in a session before doing a submodule update and a master update it's likely you'll cause problems. I wonder if I can write some kind of batch script or something where I write in "gitrdun" (Heh .GIT r dun...) and now I can code.

Be even better if I can code then do the same and everything is resolved then a commit and push are made...but that's not realistic...

A submodule update is only needed, if the submodules have been changed. That happens rarely and I usually add a warning in one of the commits (its generally a good idea to read the commit messages in my master branch to keep up to date).

You don't need to merge in my master changes before every commit. Indeed you shouldn't. Just once in a while and ideally once before a pull request (that is optional though).

Edit: You should always check the status before a commit anyway. Any missing submodule updates will show up there in form of unexpected changes to the submodules (OpenEngine and mangle).

EDIT: Oh, okay. I guess I was just unlucky to jump in that time since I started fresh very recently.

Alrighty - most of it is done save the actual moving in Player - no code insight in Notepad - will work on getting another project file CMaked (hopefully) soon so I don't have to keep asking where stuff is.

All I need is to be able to access the player's correct speed and uncomment the lines in exectueAutoMove() - rest should be as you outlined.

Ummm. I'm starting to throw in comments to make the code easy on the eyes - at least in the functions I write I'll be adding MSVC params for Code Insight - if I knew the equivalent statements for Eclipse (lol C++ in eclipse) or another IDE I can do that, too.

EDIT: Nevermind the Extra capital in the commit, I fixed it.

You should always check the status before a commit anyway. Any missing submodule updates will show up there in form of unexpected changes to the submodules (OpenEngine and mangle).

I feel this is an important part, control and choose what changes you stage and commit. This helps you to not commit changes you did not intend to do.

in the functions I write I'll be adding MSVC params for Code Insight - if I knew the equivalent statements for Eclipse (lol C++ in eclipse) or another IDE I can do that, too.

Are you considering doxygen as well?

You should always check the status before a commit anyway. Any missing submodule updates will show up there in form of unexpected changes to the submodules (OpenEngine and mangle).

I feel this is an important part, control and choose what changes you stage and commit. This helps you to not commit changes you did not intend to do.

in the functions I write I'll be adding MSVC params for Code Insight - if I knew the equivalent statements for Eclipse (lol C++ in eclipse) or another IDE I can do that, too.

Are you considering doxygen as well?

I'm not very knowledgeable about how to do it for DOxy, but if you know any good resources or if it's easy to do I can do that.

Also - If I do that, if there's any particular sections you guys want done first, just say so, no sense in just going top to bottom - not ALL of it needs the love.

Bouncing Ideas for Sneak and NPC awareness:
Sneak is more involved because it requires the use of a skill that can be broken if the player is spotted.

Some thinking:

For every NPC currently loaded in a cell, take that array and pass it to a checkAwareness() method in player, which checks all of them. If the PC is spotted the sneak effect does not get applied.

Now, this would end up being called all the time, and requires having NPCs loaded in an array for each cell loaded...and that means there could be a lot very far away that may not need to be loaded.

Also - you can't just check a cone (or any area around the NPC) of visibility by itself, since the cone would also be doing checks on objects in front of the line from them to the player.

Hmmm...don't really have any other ideas ATM. Maybe it really should be left for later when we have more stuff for NPCs in place.

Hmmm...don't really have any other ideas ATM. Maybe it really should be left for later when we have more stuff for NPCs in place.

As I said before, we are not ready to implement sneak. It needs other features, that are not implemented yet.

Hmmm...don't really have any other ideas ATM. Maybe it really should be left for later when we have more stuff for NPCs in place.

As I said before, we are not ready to implement sneak. It needs other features, that are not implemented yet.

No problem - anyways, I'm playing with making the Player walk. I implemented that based off the current movement and what I've learned so far. Will clean it up if it's good and tests well in game when the speed values become more sophisticated.

Anyways, this is all that's left :

Code: Select all

//TODO: Make player go. 
      /*Access moveZ, access walking/running speed, -1 is for correct direction, 
      otherwise you end up running backwards - not a bad idea for a future feature, actually...*/
      /*
      if (this.misWalking == false) 
      {
      //inputmanager.moveZ = 300 * duration * -1;  
      } else {
      //inputmanager.moveZ = 100 * duration * -1; 
      }
      */

Looking good? I don't think I can access MoveZ this way, but otherwise this is all that's left.

Sorry, no. The movement logic is totally skewed. Also, the code has syntax errors (you are supposed to build before you commit) and there are uninitialized variables (very bad in C++). Furthermore you are going way beyond the scope of the simple auto-run, which is not a good idea at this point, because you are dealing with lots of placeholder code, that will be ripped out later anyway (e.g. the whole speed stuff has absolutely no reason to be in the InputManager).

I gave you a precise description how to accomplish the task. Why aren't you following it? I suggest you go back and start from scratch (or from your 2nd commit in the branch, which was still okay).

Sorry, no. The movement logic is totally skewed. Also, the code has syntax errors (you are supposed to build before you commit) and there are uninitialized variables (very bad in C++). Furthermore you are going way beyond the scope of the simple auto-run, which is not a good idea at this point, because you are dealing with lots of placeholder code, that will be ripped out later anyway (e.g. the whole speed stuff has absolutely no reason to be in the InputManager).

I gave you a precise description how to accomplish the task. Why aren't you following it? I suggest you go back and start from scratch (or from your 2nd commit in the branch, which was still okay).

I can understand how you think that a lot of the current implementation (project wide) is going to be ripped out as is, and making more of it to be ripped out seems a bit redundant and useless, but I wrote methods for player with that in mind. If we take that to the ultimate conclusion then noone should be doing anything because so much of it has to be ripped out and done right later. You're setting me up to fail from the beginning. I knew this going in, but hey, at least someone's on the job.

What of the movement logic? Skewed? I can see how you could flip the conditions and executions around and get the same result. My code is readable and logic pretty simple - if there's something in the logic that won't make the player go forward, then explain it to me so I can fix it. I'm doing this according to what someone else wants, so you have a better idea of what you want than I do. You may not want to explain it to me, but I can't carry out your own design if you're leaving your design up to me. I have my own ideas for doing this, but I'm fine with doing what you want - you will, however, have to take time to explain it, as much as you feel you shouldn't have to.

You did give me instructions, and I followed them. every point of your list was done. You left it up to me, I posted as I went, and I didn't have any feedback, so I continued.

I have to say I'm a little upset that you recommended a task for me only to say I didn't carry it out to your own specifications given what was given to me, especially when you already feel it's not worth doing to begin with due to the fact it all is going to be ripped out anyways.

Is this the kind of response I can expect every time? If you want, I can go over each step you gave me, and give you the corresponding piece of code I wrote for it - you will say you should have done it yourself, and I won't have any response but to agree, then observe that you have a lot of work to do on this project.

I apologize if I didn't telegraph the code I added because of procedure. I appreciate the reminder of the memory stuff - my latest commit fixed the initialization of the two booleans, the others are commented out.

As for all the speed variables in inputmanager - it was how you were moving when I got there. I didn't change how you already were moving. None of your instructions mentioned scrapping your current moving system, although I would have if you wanted me to. I preserved what was written and wrote my code in the context of that being already correct.

Now - if you'd like, you can continue cutting me down, but I think I've humbled myself before everyone enough during this thread. If there's something wrong, I'll will fix it, If everyone would like me to delete the entire thing, I will.

I'm sorry for the wall of text, but I really do feel like you have no trust in me. I had to write it because this problem appears as something I hear about and see in daily life both professionally and domestically, and I definitely want to overcome it and be prepared to deal with it.

You did give me instructions, and I followed them. every point of your list was done.

That exactly is the problem. You followed them up to #3 and than you did something completely different. I gave you these instructions exactly because there is so much placeholder code, that will require changing. By following them you would largely have avoided your work going to waste.

I am not cutting you down and I didn't set you up for a fail. This is a review of your code based on technical merits and the given specifications. Nothing more, nothing less. I am sorry, if you can't take valid criticism, but that isn't really my problem.

I have a few more minutes time while I wait for something else to compile (not OpenMW).

In your executeAutoMove function you are trying to access local variables of a function, that belongs to InputManager. That makes no sense at all.

And neither does executeAutoMove, really. What we will do is call a player update function regularly (once per frame). This function will check if the auto move and or manual move flag (not implemented yet) is set and if so, will move the player forward by a factor determined by the length of the previous frame multiplied by another factor generated from player stats (currently a constant).

Edit:

I'm sorry for the wall of text, but I really do feel like you have no trust in me

I think this topic is better handled via PM.

Bad air aside, we should resolve this. Honest and truthfully, I'm a transparent guy - and teams shouldn't be afraid of conflict, nor should they shy away from bad air if it ever arises.

I can take constructive criticism. If you help me walk away from a conversation able to go in and say, "ah, there. Fixed", then I'm fine with that. When you pointed out the syntax I went back and had to attempt to build the project in MSVC - I checked if there were any red squigglies - none. Strange - where is the syntax error. No errors until I build, and it yielded an error in instances when I used "this".

Now, I walked away understanding what you mean - but I would have reacted much better if you said "even if it doesn't successfully build, there are errors you just find when you build.". That would made your point entirely clear and we both would have walked away better. Why didn't you say so?

4. Now add a function pulse (float duration) to Player (duration is the time since the last call in seconds). Check if auto run is set, call the movRel function of Player. You want X=0 and Y=0. Set Z to a suitable factor multiplied by the duration multiplied by -1. You will have to experiment a bit here, but don't spend too much time on finding the right factor. It will be replaced, once we have more game mechanics in place.

When I first read this, I had no idea why you named it pulse() - it didn't explain the functionality to me, and the name just had no meaning. However, since I didn't have access from player to MoveX, MoveY, and moveZ (the ones that did the work), which are defined in inputmanager, and don't seem to be pointing anywhere, I named the function something meaningful - executeAutoMove() - the past few days I've had it posted the day I wrote it to show you. You've said nothing.

What I will admit is that I probably should have asked you about pulse in more detail, but I expected a bad response. I decided to figure it out on my own. Sorry.

5. Finally call the pulse function from Engine::frameStarted. You can get the duration value from the function argument. Here is the API: http://www.ogre3d.org/docs/api/html/str ... Event.html

This was done, with the exception of the ogre element I call then cast, which I included a comment with. After looking at it, I decided to cast it as float and send it on its way - comment with the problem included. Nothing.

calls to local inputmanager vars - see previous posts. I asked about that twice.

Hold on - lemme go over your latest post carefully before I continue. We'll start over.

In your executeAutoMove function you are trying to access local variables of a function, that belongs to InputManager. That makes no sense at all.

And neither does executeAutoMove, really. What we will do is call a player update function regularly (once per frame). This function will check if the auto move and or manual move flag (not implemented yet) is set and if so, will move the player forward by a factor determined by the length of the previous frame multiplied by another factor generated from player stats (currently a constant).

Okay...let's start over from pulse()

MoveX, MoveY, and MoveZ are all declared, initialized, and operated on in inputmanager - they don't point to anything a player has, so it has to be called in a function elsewhere where I can make a value and pass it to that function that somehow belongs to player.

player.moveRel(moveX, moveY, moveZ);

There. That's the one I want. That clears up that. I can completely change how I do it, now. I wish I had found that earlier.

As for the stats - they don't exist, yet. For now, executeAutoMove() can use constants, multiply them by the duration parameter (the frame, not the event), and then -1 (forward is moving by Negative X).

Does this sound good?

Now, I walked away understanding what you mean - but I would have reacted much better if you said "even if it doesn't successfully build, there are errors you just find when you build.". That would made your point entirely clear and we both would have walked away better. Why didn't you say so?

For me building is the only way to verify syntactic correctness. I don't use any other helper function like you describe and I was not aware of their deficits.

I named the function something meaningful - executeAutoMove() - the past few days I've had it posted the day I wrote it to show you. You've said nothing.

I admit, that I didn't pay much attention. Was busy with other stuff.

What I will admit is that I probably should have asked you about pulse in more detail, but I expected a bad response. I decided to figure it out on my own. Sorry.

Yes, asking would have been the right decision here. And no, that would have no resulted in a bad response. But no problem. Mistakes happen.

When I first read this, I had no idea why you named it pulse()

Called in regular intervals. Like a heartbeat?

There. That's the one I want. That clears up that. I can completely change how I do it, now. I wish I had found that earlier.

Reading the relevant header files is usually a good idea. Usually you can go for the doxygen documentation too (link on the wiki). In this specific case, that wouldn't have helped though. The Player class is a recent addition (result of PlayerPos refactoring). The doxygen documentation is still on 0.9.0.

As for the stats - they don't exist, yet. For now, executeAutoMove() can use constants, multiply them by the duration parameter (the frame, not the event), and then -1 (forward is moving by Negative X).

Negative Z, unless I am mixing that up too.

We are getting closer. Just to make it clear: The input manager has no business in moving the player around (the regular WASD keys aren't handled good yet either). We want all the movement stuff encapsulated as much as possible, so we don't have much work when generalising the code for NPCs. All the input manager has to do is to configure the player object appropriately, which then will be update everything each frame via the function called from the framelistener.

Meant Z - sorry.

Tomorrow I'll redo it and post.

please discuss about that in pm

please discuss about that in pm

Don't worry - All's cool now.

Okay - based off yesterday - I changed it up...

I'm still not sure if you're saying if you want to scrap the way the player is moved in inputmanager so I can send all that to player.. If you want me to do that I can make the functions for WASD movement and move it out of inputmanager for good. It looks like you still want to let input manage get some sort of values and then a moverel is called if certain polled keys are pressed. Anyways, they handle walking and running. When speed calculations are implemented they'll need to be changed.

Anyways, I changed the pulsing function.

Is this better?

I built - no errors came up in the two files.
commited and pushed.

Here's the function :

Code: Select all

   //NOTE: we don't have speed being calculated yet, so for now this function only requires a frame duration.
   /// <param name="duration">float value representing time since last call</param>
   void executeAutoMove(float duration) 
        {
        if (mAutoMove == true) 
      {
      //if player is running
         //Make player go at full speed
         //player.moveRel(0, 0, (300*duration*-1)); 
      //else go forward at walk speed.
       }
   }

Good?

Mostly. If you want you can continue with the movement control refinements, but lets finish this up first.

I guess the "player." in "player.moveRel(0, 0, (300*duration*-1)); " was just a relict from refactoring? Because you don't need it here. Also moveRel in its current state will not accept literal values. You need to use variables.

Your indention is a bit chaotic in some places. Further more there is a mixup with the naming conversion. If you have a member variable mName, the get function is usually called getNme, not getmName.

Other than that, from a first visual inspection, I have no objections. Will give it a test run, once you have finished it up and then we can talk about improving it further.

Mostly. If you want you can continue with the movement control refinements, but lets finish this up first.

I guess the "player." in "player.moveRel(0, 0, (300*duration*-1)); " was just a relict from refactoring? Because you don't need it here. Also moveRel in its current state will not accept literal values. You need to use variables.

Your indention is a bit chaotic in some places. Further more there is a mixup with the naming conversion. If you have a member variable mName, the get function is usually called getNme, not getmName.

Other than that, from a first visual inspection, I have no objections. Will give it a test run, once you have finished it up and then we can talk about improving it further.

yeah, the indentation is weird - I've been working in notepad, enforce the standard, then open in MSVC or if you look at my git commit - it's not. It's kinda frustrating because even if I stick to one or another it never looks right. I find pasting code in forums also never looks right.

I'll have to stick with MSVC - since I have to build to check for hidden errors anyways.

Ah - yeah. I don't need to reference player to use that function. I'll delete that.

It doesn't take literal values?
What I'll just do is calculate a variable and send it to the function instead.

okies - latest commit and push:

https://github.com/Star-Demon/openmw/co ... c2f04a421a

Tested. The toggle functions (in InputManager) are wrong. You are always setting the mode, while you should invert it instead.

Your formatting problems are a result of mixing spaces and tabs. As I said before, don't! Check your editor settings. Something, somewhere is wrong.

The function names still need to be adjusted (setAutoMove instead of setmAutoMove).

Tested. The toggle functions (in InputManager) are wrong. You are always setting the mode, while you should invert it instead.

Your formatting problems are a result of mixing spaces and tabs. As I said before, don't! Check your editor settings. Something, somewhere is wrong.

The function names still need to be adjusted (setAutoMove instead of setmAutoMove).

Huh. Guess as convention you don't name the get and sets after the literal name of the variable. Okay to drop the m function names and elsewhere, then? Got confused. Sorry. Anyways - that's a quick and easy fix. Done. Toggle function toggles, too. That should be it.

I guess I rushed into this whole thing, so I ended up doing a lot more work than I had to. I really could have done better.

Does not compile for me (you didn't change all instances of the function name). Remember, always build before comiting!

Anyway, here is our further roadmap for this feature:

- You finish up your current work.

- I create a new topic branch, in which I do the backend work on the movement modes.

- Once you are done, I merge your master branch into my movement branch. I fix the submodule hiccup and then you immediately merge my movement branch back into your master branch.

- Then we do some more improvements on movement control and player stances (I think with my upcoming backend-changes we actually can implemented sneak partially, just not the detection stuff).

Is that acceptable to you?

Does not compile for me (you didn't change all instances of the function name). Remember, always build before comiting!

Anyway, here is our further roadmap for this feature:

- You finish up your current work.

- I create a new topic branch, in which I do the backend work on the movement modes.

- Once you are done, I merge your master branch into my movement branch. I fix the submodule hiccup and then you immediately merge my movement branch back into your master branch.

- Then we do some more improvements on movement control and player stances (I think with my upcoming backend-changes we actually can implemented sneak partially, just not the detection stuff).

Is that acceptable to you?

Sounds good. By player stances you mean Standing, casting, Brandishing, crouching?

I missed one? one sec. I see - I missed the two in the toggle in inputmanager. That's fixed.

I think that we should fully implement the calculation of all (or most) player stats at the base so we can implement all the game stuff that relies on it. Otherwise Player is just an incomplete data class (with some fancy stuff), and why have a half finished data class?

Sounds good. By player stances you mean Standing, casting, Brandishing, crouching?

Running, Sneaking, Combat (and normal, which would be the absence of all stances). We can address casting later (not ready for it yet and it will be nearly the same as combat anyway).

I missed one? one sec. I see - I missed the two in the toggle in inputmanager. That's fixed.

Looks good. I will merge your code today. I will also fix the submodule and the tab problems. But please make sure, that they don't come up again.

I think that we should fully implement the calculation of all (or most) player stats at the base so we can implement all the game stuff that relies on it. Otherwise Player is just an incomplete data class (with some fancy stuff), and why have a half finished data class?

We are not ready for that yet (missing research only being one of the things that is stopping us). And this stuff won't go into the player class anyway. That would be wrong, since NPCs and the player are using the same game mechanics.

We are not ready for that yet (missing research only being one of the things that is stopping us). And this stuff won't go into the player class anyway. That would be wrong, since NPCs and the player are using the same game mechanics.

that's true - actually, when I look at my stuff, I have:

Code: Select all

-Actor
--Character
---NPC
---PC
--Creature

This lets me treat PCs and NPCs much more similarly by building on character. Not sure if that's what we're doing - I haven't looked.

Okay. Done. Please merge my movement branch into your master branch and then do a submodule update. That should fix all the problems.

Okay. Done. Please merge my movement branch into your master branch and then do a submodule update. That should fix all the problems.

merge went fine

git submodule update

FATAL: reference is not a tree: (Hex string )
unable to check out (same hex string) in submodule path libs/mangle

Never seen this one before.

Maybe try
git submodule sync

and then another update? Currently I have no other idea how to address this problem.

Never seen this one before.

Maybe try
git submodule sync

and then another update? Currently I have no other idea how to address this problem.

Okay, that went much better. That should do it.