GamepadHandler patch

Coordinator
Jan 25, 2008 at 3:09 AM
Ok, I've submitted the GamePadHandler....finally.

It should be fairly easy to review.
Jan 25, 2008 at 6:23 AM
Generally:
  • You should use the .patch extension on submittede patches
  • Remember to use this prefix on instance members
  • Be consistent in the xml comments, i.e. the gameTime parameter should use the same comment as everywhere else.
  • Remember to provide xml comments even for private methods, we are not the only once reading the code

In Configuration.cml
  • Remember to use 4 spaces as indent

In FreeCamera.cs
  • You are adding modifiers and inverting to the camera, this should most likely be in the handler as this code would be needed for all cameras?
  • Don't use short names like Rot, use Rotation (never know if you actually mean Rotation or Rotting)
  • In Game_GameMessage you throw the wrong exception type
  • In Game_GameMessage do not use brackets around case statements, it's implicit
  • In HandleThumbsticks when calculating right thumb you are first setting invertMod then multiplying by(-invertMod) why not set invertMod to the correct value?

In GamePadDataTypes.cs
  • Remember to have each type in a seperate file, if you need to group create a folder
  • You have both GamePadTriggerType and GamePadThumbstickType, I think that just having a single Side enumeration would be enough.
  • Do not offset indexes by 1 unless you have a special need to do so, not even for enums.
  • Remember to add xml comments on enumeration values as well (Yes VS actually uses these)
  • Remove unneeded using statements

In GamePadHandler.cs
  • The states fields can be made readonly as they are only assigned to at construction
  • Do not use enumerations as values for allocating arrays as you can't control the actual value. In this case it even wrong as PlayerIndex.Four == 3
  • On UpdateCore do not use the enum in the for loop, if the values change you will generate exceptions, best solution is to get the enum values and loop over those
  • There is no reason to pass current and previous state, they are fields, just pass PlayerIndex
  • ProcessButtons can be split up into smaller methods, which would remove a lot of code
  • On Send... all have similar code you should rework this and reduce the code.

In MessageType.cs
  • On ButtonHeld I think you should use ButtonPress similar to keyboard events, as it's the same state (Something being pressed).

I think that instead of having a struct defining each message type and then using Message<XXX> if would have been clearer to use specific messages which were inheriting from Message<XXX>. This way you can create something like:

public class GamePadButtonMessage : Message<Buttons>
{
    private PlayerIndex playerIndex;
 
    public PlayerIndex PlayerIndex
    {
        ...
    }
} 
And then when getting the message:

    GamePadButtonMessage gpMessage = .....
    if (gpMessage.PlayerIndex == PlayerIndex.One)
    {
        switch (gpMessage.Data)
        {
            case Buttons.A:
                break;
        }
    } 
Coordinator
Jan 25, 2008 at 2:28 PM

Sturm wrote:
  • You should use the .patch extension on submittede patches

I thought the extension was put on automatically.


Sturm wrote:
  • Remember to use 4 spaces as indent

My file had different indentations, I was just following the pattern.


Sturm wrote:
In FreeCamera.cs
  • You are adding modifiers and inverting to the camera, this should most likely be in the handler as this code would be needed for all cameras?

You probably wouldn't to invert all values of the control's thumbstick Y Axis, as it could be used to things like menus or other stuff in a game. Likely you would just want your view inverted. This may be something that ALL cameras use, but I don't think you want to invert the value in the handler.


Sturm wrote:
  • In Game_GameMessage you throw the wrong exception type

I copied and pasted the exception types that were already in use. Maybe I had old code.
{quote}


Sturm wrote:
  • In Game_GameMessage do not use brackets around case statements, it's implicit

But brackets look better. :)


Sturm wrote:
  • In HandleThumbsticks when calculating right thumb you are first setting invertMod then multiplying by(-invertMod) why not set invertMod to the correct value?

Because I wanted the way invertMod was calculated to be consistant. This way people know that the values you get from the controller itself are inverted by default.


In GamePadDataTypes.cs
  • Remember to have each type in a seperate file, if you need to group create a folder

I disagree. In fact, I didn't want to even create a single file type. In my opinion all the data types should be put inside of a region at the bottom of GamePadHandler.cs. I would really like to prevent hundreds of files due to messages, I believe all related messages should be grouped together.


Sturm wrote:
  • You have both GamePadTriggerType and GamePadThumbstickType, I think that just having a single Side enumeration would be enough.

I thought about that as well, I can submit that on my next patch.


Sturm wrote:
  • Remember to add xml comments on enumeration values as well (Yes VS actually uses these)
  • Remove unneeded using statements

I thought all my enums had comments.
I don't remember having any unneeded statements. In fact, I thought I put a comment as to why they were needed. I don't have the code in front of me so I can't confirm this.


Sturm wrote:
In GamePadHandler.cs
  • The states fields can be made readonly as they are only assigned to at construction

The current and previous pad states? They're assigned to every loop if a control is plugged in.


Sturm wrote:
  • Do not use enumerations as values for allocating arrays as you can't control the actual value. In this case it even wrong as PlayerIndex.Four == 3

I don't agree that using an enum is bad, it makes it very readable and easy to understand, however in this case you are correct. I will simply use 4.


Sturm wrote:
  • On UpdateCore do not use the enum in the for loop, if the values change you will generate exceptions, best solution is to get the enum values and loop over those

Can an enum value change?


Sturm wrote:
  • There is no reason to pass current and previous state, they are fields, just pass PlayerIndex

Agreed I'll fix this on my next patch.


Sturm wrote:
  • ProcessButtons can be split up into smaller methods, which would remove a lot of code

I'm all ears, I certainly didn't want a method that big, that was as small as I could get it because I could not find a common way of looping through all buttons, especially as some IF statements refer to ButtonStates, but the IsButtonUp/Down methods refer to GamePadButtons.


Sturm wrote:
  • On Send... all have similar code you should rework this and reduce the code.

I didn't think about that. That is what reviews are for I guess :). I'll look into fixing that on the next patch.


Sturm wrote:
In MessageType.cs
  • On ButtonHeld I think you should use ButtonPress similar to keyboard events, as it's the same state (Something being pressed).

Something being pressed just don't like something is Down. To me, held is much more descriptive.


Sturm wrote:
I think that instead of having a struct defining each message type and then using Message<XXX> if would have been clearer to use specific messages which were inheriting from Message<XXX>.

I thought about this as well. If we'd all like I can do that on my next patch.

Thanks for the review. I won't have time to submit another patch for a few days at the earliest. If you guys want to use what we have for 0.19 I'm fine with that, and I'll submit the patch soon after we start 0.20. Or if you guys want to wait a few days to a week for my changes that is fine too. I don't really care either way.
Jan 26, 2008 at 7:03 PM

LordIkon wrote:
I thought the extension was put on automatically.


It's not but I can make the custom build apply a .patch extension if none was set.


LordIkon wrote:
My file had different indentations, I was just following the pattern.


I think we should always use the same indentation in all files as this gives the best experience when viewing our code.


LordIkon wrote:
You probably wouldn't to invert all values of the control's thumbstick Y Axis, as it could be used to things like menus or other stuff in a game. Likely you would just want your view inverted. This may be something that ALL cameras use, but I don't think you want to invert the value in the handler.

Because I wanted the way invertMod was calculated to be consistant. This way people know that the values you get from the controller itself are inverted by default.


You are right, but we most likely should think more about this, I certainly do nok like the math used first doing -1/1 and then multiplying by the inverse, I would have liked the initial assignment to be the right one.


LordIkon wrote:
I copied and pasted the exception types that were already in use. Maybe I had old code.


Because you copied it you forgot to change the text in the thrown exception.


LordIkon wrote:
But brackets look better. :)


It just adds indentation and doesn't do anything for code clerity.


LordIkon wrote:
I disagree. In fact, I didn't want to even create a single file type. In my opinion all the data types should be put inside of a region at the bottom of GamePadHandler.cs. I would really like to prevent hundreds of files due to messages, I believe all related messages should be grouped together.


If you have hundreds of messages inside the same file you are going to have a lot of updates to the same file and you are mostly risk that two people are modifying the same file. In a worst case you might accendentially override local changes to that files because it was updated on the server, having everything seperated makes this a lot less likely.


LordIkon wrote:
I thought all my enums had comments.


The enums do, but not the enum values.


LordIkon wrote:
I don't remember having any unneeded statements. In fact, I thought I put a comment as to why they were needed. I don't have the code in front of me so I can't confirm this.


VS automatically adds 3 using statements, mostly System.Text and System.Collection are not needed.


LordIkon wrote:
The current and previous pad states? They're assigned to every loop if a control is plugged in.


They are not assigned to just repopulated.


LordIkon wrote:
I don't agree that using an enum is bad, it makes it very readable and easy to understand, however in this case you are correct. I will simply use 4.
...
Can an enum value change?


Using enums isn't bad at all, I'm not disarguing that. But using thier integer value in a for loop you have to be very careful. Yes the enum values can certainly change, not runtime but in the next release or hotfix. The Xna team might decide to change the behavior so that it a flag and have the values 1,2,4,8 or something else.


LordIkon wrote:
Something being pressed just don't like something is Down. To me, held is much more descriptive.


But you are not holding the button you are pressing it down.
Coordinator
Jan 27, 2008 at 3:10 AM
Yes, but pressing it very quickly and releasing is still pressing it down. Holding it down implies you're not releasing it.

Also, some very good points in there, I will fix these on my next patch. Thanks for the review. Are you going to commit it as is or would you rather wait a few days for another patch?
Jan 27, 2008 at 12:14 PM

LordIkon wrote:
Yes, but pressing it very quickly and releasing is still pressing it down. Holding it down implies you're not releasing it.


No it's not, you can't both be pressing the button and releasing it at the same time.

I don't know why we even are arguing about this. It's standard naming conversion on the windows platform (and others) and we are just confusing others by introducing a new name for something which is already defined (Remember that the function you call is also named getpressedkeys)


LordIkon wrote:
Are you going to commit it as is or would you rather wait a few days for another patch?


As the patch contains errors I would rather wait until a new patch is ready.
Coordinator
Jan 27, 2008 at 7:37 PM
I agree you cannot be pressing and releasing it at the same time. But if we want to get specific about it, then the button is never actually held down, it is simply in a down/pressed state for multiple frames at a time.

Pressed doesn't sound any different than down. In fact, if you used to the word pressed I would assume you meant for one frame. When you tell someone "I pressed the button" they're not going to assume you "pressed the button and held it in". If you say the button is down, I'm also going to assume it is down for that frame, but make no assumption that it was released. However, if you were to tell me the button was "held", then I know, without a doubt, the button is not being released immediately.

I don't see how we're confusing anybody, nobody is using 0.19 yet, so whatever we set forth soon we'll be what they're "used to". If anything, I was confused we used both pressed and down, and why we didn't use something like held.

Just because windows sets a convention means nothing to me. Windows is not the universal gaming platform, and I feel we're catering more to the gaming community and their conventions than we are to Microsoft's conventions. I don't feel like building an engine based specifically on .net and/or Microsoft conventions and standards just because we're using C#. There needs to be more reasoning behind it than that. What I am concerned with is what makes this engine easy to get ahold of and develop something with in the shortest amount of time.
Coordinator
Jan 27, 2008 at 7:44 PM
Oh, and I'll try and get the next patch in by Tuesday night.
Jan 28, 2008 at 2:28 AM

LordIkon wrote:
I don't see how we're confusing anybody, nobody is using 0.19 yet, so whatever we set forth soon we'll be what they're "used to". If anything, I was confused we used both pressed and down, and why we didn't use something like held.


It's not confusing those using the engine, but all those who are going to using it will be puzzled about it (I was) as the norm would be Up/Press/Down.


LordIkon wrote:
Just because windows sets a convention means nothing to me. Windows is not the universal gaming platform, and I feel we're catering more to the gaming community and their conventions than we are to Microsoft's conventions. I don't feel like building an engine based specifically on .net and/or Microsoft conventions and standards just because we're using C#. There needs to be more reasoning behind it than that. What I am concerned with is what makes this engine easy to get ahold of and develop something with in the shortest amount of time.


It's not a Microsoft norm it is the defacto norm, have a look at JavaScript and various Linux apps as well. And yes we most certainly should use the same patterns as Microsoft does where appropriate, as everyone coming to this engine would be expecting this behavior. We are developing on a specific platform (Microsoft) and using a speciic framework (.net/Xna) so there is very good reasons to use the same terms as those. In fact I would require a very good reason in those cases where we do not follow those patters.

But if you do not want to use that specific pattern and want a more Linux/Unix way of doing it you should do it properly and use the following names:

ButtonPress
ButtonHeld
ButtonRelease

As those coming from that world would already be familiar to Press/Release.

Try to post this on the xna forums and see what others think about this.
Coordinator
Jan 28, 2008 at 3:17 AM
That is a good idea. I'll leave the question completely unbiased. Thanks for the idea.
Coordinator
Jan 28, 2008 at 3:52 AM
Ok, I've posted the question up on the forums, we'll wait a day or two and see the results. I have a couple of days till I put my next patch through anyway.

http://forums.xna.com/thread/43486.aspx
Jan 28, 2008 at 5:34 AM
Ok, I can see the reasoning for held (I actually had to look up the events on MSDN, as I'm not a WinForms dev) and reading the definitions on the events I can see that we need to have:

Down
Held
Up

I'll do the rework for the keyboard and I'll check it in later.
Coordinator
Jan 29, 2008 at 2:56 AM
Ok, I've re-submitted the gamepad handling patch. Somebody please review this thing so we can get on with a 0.19 release :)
Jan 29, 2008 at 4:45 AM
In GamePadDataTypes.cs
  • You should use CamelCase on public fields, since this will ease code rework when moving these to properties
  • Most of the types should not have writeable fields, Most should have readonly properties.
  • I think they should inherit from IPoolItem as you are going to create a lot of these over the game cycle, but only need a few.
  • GamePadInputSide is still missing xml comments on enum values

In GamePadHandler.cs
  • You still have an issue with "GamePad.GetState((PlayerIndex)i).IsConnected" as you can not be sure that PlayerIndex will always have the values 1 to 4
  • On ProcessThumbsticks you have an issue if the user moves the stick very slowly, as this will not raise any message nomatter how far he moves, you should accumolate the values instead.
  • You really need to rework the ProcessButtons method as there is too much duplicated code

In SceneManager.cs
  • Nice rework

I can help you rework the ProcessButtons if you need help on that.
Coordinator
Jan 29, 2008 at 4:49 AM
Any suggestions for ProcessButtons would be great. I'd have to pass two states, and a button state and button as well from what I can tell.
Coordinator
Jan 29, 2008 at 4:51 AM
I think I got a good way to re-work it, give me about 30 minutes and I'll submit a new patch.
Coordinator
Jan 29, 2008 at 5:29 AM
I'm about to submit a new patch, here are my notes:

  • Couldn't make items readonly because then they could not be assigned to.
  • I did make the structs IPoolItems
  • Added xml comments where missing

  • "GamePad.GetState((PlayerIndex)i).IsConnected" requires an cast as an enum, because it will only accept a PlayerIndex as input. If we're restricting the for loop to a valid range then this cannot cause a problem. The problem is if someone tries to alter the for loop. I've made a const value for the loop.
  • I cannot reproduce the issue you are experiencing with moving the sticks very slowly. There are posts from Shawn Hargreaves about a lack of Gamepad response near the 0.0 range of the thumbsticks. I'll post the link if I find it.
  • I re-worked ProcessButtons.

Additionally, I've added support for video cards that support only 16-bit index buffers. You'll see some changes to Terrain, TerrainPatch, and QSConstants. Most of this is preliminary, and we cannot fully test it unless someone has a pretty old video card or integrated graphics chipset. However there is a post on the forums in which someone got something just like this working in 0.182b on their 16-bit IB card.

I'll post the patch in a couple of minutes.
Coordinator
Jan 29, 2008 at 5:31 AM
Patch has been uploaded.
Jan 29, 2008 at 6:00 AM

LordIkon wrote:
"GamePad.GetState((PlayerIndex)i).IsConnected" requires an cast as an enum, because it will only accept a PlayerIndex as input. If we're restricting the for loop to a valid range then this cannot cause a problem. The problem is if someone tries to alter the for loop. I've made a const value for the loop.


The issue here is that if the enum values are 1,2,4,8 your code will fail, you should use Enum.GetValues() to get the values and then loop over those.


LordIkon wrote:
I cannot reproduce the issue you are experiencing with moving the sticks very slowly. There are posts from Shawn Hargreaves about a lack of Gamepad response near the 0.0 range of the thumbsticks. I'll post the link if I find it.


You have code specificially targeted at this, so there is an issue. It might not be possible to do it easily using a controller, but if you wrote a unittest which incremented the value by float.eplison-1 about 10M times you would still not trigger a single messages. You should consider reworking this so that it uses a cumulative value instead.

In Camera.cs
  • You should never let enums start at 1, unless there is a very good scenario for it

In FreeCamera.cs
  • The text for the thrown exception is still wrong

In TerrainPatch.cs
  • Why do you allow nullable on method parameters, you should avoid this if possible. You should, most likely, create two methods

In GamePadHandler.cs
  • Shouldn't MaxNumGamepads be in QSConstants?
  • MaxNumGamepads should be MaximumGamePads or something similar, do not use short names
  • Remember to use this prefix for instance members
  • I guess that it's fine, I would have liked not to pass ButtonState as you already pass the button, but I quess this should hold off the release
  • On ProcessButton MessageTypeInt should use pascalCase not CamelCase
  • Though you did make the datatypes IPoolItems you are still not using the ObjectPool to get them. You have to look into KeyMessage to see how this is done, and use a similar approach
Coordinator
Jan 29, 2008 at 6:06 AM
Edited Jan 29, 2008 at 6:11 AM
I can touch up a lot of this stuff on my next patch, however I may not have time for that for a few days. Its up to you if you want to continue and wait for v0.19, I'm in no hurry, I just know that noone else has work to do until we start 0.20, unless they start working on their 0.20 changes now.

I used enums like that because Microsoft does the same for their gamepad stuff, referring to Microsoft.Xna.Framework.Input.Buttons. However, I do need to change the one that is 1, 2, 4, 8.

I could write a unittest to give values that small, but those are values that the controller cannot produce, so I'm not sure what the point would be. I will consider cumulative values however. Everything else I will look at.
Coordinator
Jan 29, 2008 at 6:29 AM
Crap, I forgot X & Y buttons....
Coordinator
Jan 29, 2008 at 6:35 AM
Alright, I'm submitting one final patch, then I'm off to bed and I won't be around the code for a couple of days. Patch will go up in a few minutes.
Jan 29, 2008 at 6:42 AM
The question is always will we have time to do any patching on this in 0.20 as we are working on 0.2 features? If you think this is good enough then check it in create issues for the above and fix them in 0.20
Jan 29, 2008 at 6:44 AM

LordIkon wrote:
I used enums like that because Microsoft does the same for their gamepad stuff, referring to Microsoft.Xna.Framework.Input.Buttons. However, I do need to change the one that is 1, 2, 4, 8.


Don't repeat others mistake, learn from them instead.


LordIkon wrote:
I could write a unittest to give values that small, but those are values that the controller cannot produce, so I'm not sure what the point would be. I will consider cumulative values however. Everything else I will look at.


If you can never get the values, what's the point in having the code? Don't code what you don't need.
Coordinator
Jan 29, 2008 at 6:44 AM
Unless you see something critical in the patch I just released, I'd say release and create a single issue for what remains, and assign it to me. The first thing I plan on working on for 0.20 is cleaning stuff up anyway, like the camera system.
Coordinator
Jan 29, 2008 at 6:46 AM


Sturm wrote:
If you can never get the values, what's the point in having the code? Don't code what you don't need.


True enough, I usually use float.epsilon because of rounding issues with floating points, or the chance for uninitialized values.
Jan 29, 2008 at 6:49 AM
That sounds more like the need for input validation, if you get rounding issues or uninitialized values you should throw an exception, we should not allow neither in our code.
Jan 29, 2008 at 6:52 AM
Your checkin contains the changes to the cmd files, they need to be removed before next checkin
Jan 29, 2008 at 2:14 PM
Looks good so far. The only additional comment I have is that I'm a little concerned about the use of Reflection inside a per-frame update loop. Namely, GamePadHandler.UpdateCore. I'm not sure how .NET handles GetTypes() for enums and this may not be a problem, it's just something I need to remember to check when profiling.

Speaking of which, anyone know of a good .NET profiler that doesn't crash horribly on Managed C++ code?
Coordinator
Jan 29, 2008 at 2:14 PM
Couldn't you revert them when you apply patch and commit? If not I will commit it in a day or two when I'm around the code again.
Coordinator
Jan 29, 2008 at 2:19 PM


shawmishrak wrote:
I'm not sure how .NET handles GetTypes() for enums and this may not be a problem, it's just something I need to remember to check when profiling.


I too had concerns about this. We should run some tests on the program before 0.19 release.
Jan 29, 2008 at 2:22 PM
It actually took me awhile to understand exactly what you were trying to do with the first couple of lines in UpdateCore. The reference to PlayerIndex.One, then getting its type, was throwing me off. You can collapse both of those lines to just:

    Array playerArray = Enum.GetValues(typeof(PlayerIndex));

I find that much easier to understand.
Coordinator
Jan 29, 2008 at 2:29 PM
I like that better as well. I'd never create a generic Array in C# before, nor had I ever gotten the values of an enum, so it was new to me.
Jan 29, 2008 at 2:49 PM
In all honesty I would try to get rid of the Array, for two reasons:

  1. It's not really needed. Xbox controllers are hardware limited to 4, so manually unrolling that foreach wouldn't hurt anything.
  2. Standard Arrays (non-generic) cause allocations and boxing. For instance, this is the IL generated for that foreach:

    L_0012: ldloc.0 
    L_0013: callvirt instance class [mscorlib]System.Collections.IEnumerator [mscorlib]System.Array::GetEnumerator()
    L_0018: stloc.2 
    L_0019: br.s L_004b
    L_001b: ldloc.2 
    L_001c: callvirt instance object [mscorlib]System.Collections.IEnumerator::get_Current()
    L_0021: unbox.any [Microsoft.Xna.Framework]Microsoft.Xna.Framework.PlayerIndex          !!!!!!!!!
    L_0026: stloc.1 
    L_0027: nop 
    L_0028: ldloc.1 
    L_0029: call valuetype [Microsoft.Xna.Framework]Microsoft.Xna.Framework.Input.GamePadState [Microsoft.Xna.Framework]Microsoft.Xna.Framework.Input.GamePad::GetState(valuetype [Microsoft.Xna.Framework]Microsoft.Xna.Framework.PlayerIndex)
    L_002e: stloc.3 
    L_002f: ldloca.s CS$0$0001
    L_0031: call instance bool [Microsoft.Xna.Framework]Microsoft.Xna.Framework.Input.GamePadState::get_IsConnected()
    L_0036: ldc.i4.0 
    L_0037: ceq 
    L_0039: stloc.s CS$4$0002
    L_003b: ldloc.s CS$4$0002
    L_003d: brtrue.s L_004a
    L_003f: nop 
    L_0040: ldarg.0 
    L_0041: ldloc.1 
    L_0042: ldarg.1 
    L_0043: call instance void QuickStart.GamePadHandler::ProcessGamePad(int32, class [Microsoft.Xna.Framework.Game]Microsoft.Xna.Framework.GameTime)
    L_0048: nop 
    L_0049: nop 
    L_004a: nop 
    L_004b: ldloc.2 
    L_004c: callvirt instance bool [mscorlib]System.Collections.IEnumerator::MoveNext()
    L_0051: stloc.s CS$4$0002
    L_0053: ldloc.s CS$4$0002
    L_0055: brtrue.s L_001b
    L_0057: leave.s L_0075
    L_0059: ldloc.2 

Notice the use of IEnumerable traversal, as well as the unbox.any instruction.
Coordinator
Jan 29, 2008 at 5:43 PM
Alrighty, that is how I would've preferred it anyhow. Less code, better performance, etc.

I'll try and get those changes in tomorrow.
Jan 30, 2008 at 4:25 AM
I've uploaded a example of the GamePad code, since I haven't got a game pad attached to my PC I can't really test it, though I did add unittest for it. Have a look and feel free to use any part of it you deem fit.
Coordinator
Jan 30, 2008 at 4:35 AM
I've already finished my changes, the only problem is that stupid codeplex won't actually upload my frickin 66kb file! I've tried 4 times. If it doesn't work soon I'll have to send the patch to one of you guys via email and have you upload it, or we'll wait.

Sturm, unless you feel your changes are needed now I will look over and use them in 0.20, as I've already finished the patch. In fact, I finished and was trying to upload my patch 10 minutes before you or Shaw put your patches in. Getting very old. It happens with my posts occasionally as well.
Jan 30, 2008 at 4:39 AM
Sure, likely excuse :)

I just want gamepad code somewhere in the source tree so it's not such a pain to test Xbox builds.
Coordinator
Jan 30, 2008 at 4:42 AM
Edited Jan 30, 2008 at 5:09 AM
Sounds like you're volunteering to review this thing then? It did finally upload, oddly enough I cancelled my upload after waiting 5 minutes only to find it had finally uploaded but never told my browser to quit trying.

If this thing doesn't pass review you guys can kiss it, this is the 5th input patch in 3 days. If this one doesn't pass we'll fix it in 0.20, I just want to get the ball rolling again.
Jan 30, 2008 at 4:54 AM
Hahaha. I'll get on it tomorrow, I'm tired.
Jan 30, 2008 at 5:44 AM
Edited Jan 30, 2008 at 5:47 AM
The only issues I see are the multiple invocations of GetState per frame and the unsafe prefix cast (which btw is slower than as casting, and I know that it's only useable on reference types, at least on windows) inside GamePadHandler.cs

But you can just ship it, and fix those in 0.20
Jan 30, 2008 at 2:51 PM
I was missing two assets: FourByFour something or other.

I would update currentGamePadStates in UpdateCore to reduce the calls to GetState. Other than that, looks good!

I take it we're ignoring the finer coding guidelines for this patch?
Coordinator
Jan 30, 2008 at 3:43 PM
I thought it was relatively to guidelines :)

I thought those assets would show up in the patch. Just remove them from the project please, and thank you.
Jan 30, 2008 at 4:01 PM
I just know Sturm likes his "this." :)

Unless there are any objections, I'll commit it when I get home tonight.
Jan 30, 2008 at 5:54 PM
I don't feel strongly about using this., I just feel strongly about consistency. I didn't comment on this as I already commented on it above.
Coordinator
Jan 30, 2008 at 6:09 PM
My feeling is that 'this.' should only be used on member variables/fields, not on functions.
Jan 30, 2008 at 6:29 PM
The problem is that it can get equally complicated when doing rework, and this can really affect customer code. Moving a method from static to instance or the reverse should be noticeable in all the places where it is invoked. Also static members should be prefixed with the class, as this clearly indicates the static relationship.
Jan 30, 2008 at 7:53 PM
Seriously, I'm tired of this argument and I just want to get this code checked in so we can all get on with life.

When I get home, I'll go through and adjust the code for this. and static method calls, then commit it.
Coordinator
Jan 30, 2008 at 7:58 PM
I think shaw needs to sit in the grumpy bear chair.
Jan 30, 2008 at 8:21 PM
Sorry, I'm just impatient.
Coordinator
Jan 30, 2008 at 8:39 PM
I know, its cool so am I, that is why I said after the 5th different input patch in 3 days I was done.
Jan 31, 2008 at 12:56 AM
Alright, it's in.
Jan 31, 2008 at 1:36 AM
The gamepad code works great on my Xbox builds. I can't test on Windows, though.
Coordinator
Jan 31, 2008 at 2:42 AM
Unless there are performance or garbage issues that you're noticing, could you upload the file and activate 0.19? Just don't activate it as the default.
Jan 31, 2008 at 3:52 AM

shawmishrak wrote:
Speaking of which, anyone know of a good .NET profiler that doesn't crash horribly on Managed C++ code?


How about dotTrace (http://www.jetbrains.com/profiler/) though it's not free.
Jan 31, 2008 at 12:54 PM

LordIkon wrote:
Unless there are performance or garbage issues that you're noticing, could you upload the file and activate 0.19? Just don't activate it as the default.


Might as well push through the QSActivator patch first.