Random battleground fix

    Diese Seite verwendet Cookies. Durch die Nutzung unserer Seite erklären Sie sich damit einverstanden, dass wir Cookies setzen. Weitere Informationen

    • Random battleground fix

      Hello

      I founded this project just a few hours ago and I wanted to contribute. I had no idea where to start and then I realized that I could not queue for random battleground.
      So I wrote a patch that should fix random battleground founding.
      Here is download link:

      Quellcode

      1. http://www.filedropper.com/bgrandomfix


      I included .patch file and whole BattlegroundMgr class file just in case I screwed up something while creating .patch file
      Players still do not get bonus honor from Random BG, but queuing works just fine. :)

      Dieser Beitrag wurde bereits 1 mal editiert, zuletzt von master312 ()

    • Oh welcome to our project :)

      Thank you for your contribution, let us check your patch (I'm in a hurry, so it will take some time. I hope Everfairy can check it during the day).

      If you need some more inspiration where to start, check the open issues on github. github.com/AscEmu/AscEmu/issues
      ________________________________________________________________________________________________________________
      Edit: For me it works fine. Unfortunately BGScripts needs a refactoring but your patch looks good for me.
    • Apologies for the very late patch application, it's now live on the develop branch however it currently breaks compile on linux, so I need to fix that before merging to master

      I applied your patch and then modified it to fix a few bugs and clean the code up. I also refactored a lot of the code that was in the wrong place, however unfortunately several core functions also required refactoring, so it's a bit hard to see the changes I made to your patch.

      Feel free to ask me about any specific changes, however these are the main flaws I noticed in your patch:

      1. You did not update the character db version when modifying the character table - this is reason for patch rejection as it's crucial to ensure the database is up to date before trying to use it
      2. There were a lot of issues with the GetRbgBonus function which I'll outline in a moment (but thank you for documenting it with doxygen)
      3. I'd consider renaming the variable in the conf file for honor points and arena points as it's not immediately clear that the value given for honor is a multiplier, whereas the value given for arena is a constant
      4. (I don't really blame you for this seeing as it's not documented, but it's still an issue) Not handling the battleground list packet correctly - isRandom

      As for the GetRbgBonus function...

      1. Variable "pl" is poorly named - "plr" is the accepted shorthand for player
      2. Function is poorly named - "get" implies a few things, most notably that
      a. the function will not modify anything (this however is not a guarantee, use a const modifier for that)
      b. the function will return the value you are asking for

      To implement GetRbgBonus as a "Get" function would mean returning some kind of array or struct, or splitting each result into its own function. I agree with your design choice to supply all variables at once and have the function modify them, however that's indicative of a "Fill" or "Build" function rather than "Get"
      3. Using pointers where not appropriate - whenever you supply a pointer, the function using it has to verify the pointer is not nullptr, which leads to pointless boilerplate and crashes if people forget to check it. Use references instead of pointers where possible. It's also worth noting that you can't supply a reference as a default argument, which means when building the packet using only honor points for winning, honor points for losing and arena points for winning, you still have to declare a variable to store arena points for losing. I consider this to be a minor inconvenience compared to the headache of dealing with pointers.
      4. Passing in an entire object where not necessary - You passed in the player object when all you needed to know was whether the player had won a battleground that day and what their level was. If you only need access to an object's data, consider just passing in the data rather than the entire object - it makes the function much easier to test and refactor.
      5. Writing a function knowing that it can cause a server crash - this is never, ever okay. If a function receives bad input it should throw an exception or silently return (depending on context), not just try to use it anyway. I appreciate you documenting this, but it's still important to make sure functions fail safely.

      That's all I can think of off the top of my head, I'm sure I could nitpick some more but honestly, the patch was mostly fine - almost all of the issue were in a single function that was designed to be reasonably consistent with existing functions. It's just a shame the existing functions are such a mess ;)

      Thank you for your patch, we greatly appreciate you taking the time to contribute to AscEmu.
      AscEmu is a place for learning - don't be afraid of criticism, we don't bite <3

      Remember you can join us on Discord by clicking this invite link: discord.gg/2YJSg5M
    • <p>

      Evairfairy schrieb:

      Apologies for the very late patch application, it's now live on the develop branch however it currently breaks compile on linux, so I need to fix that before merging to master</p>

      <p>I applied your patch and then modified it to fix a few bugs and clean the code up. I also refactored a lot of the code that was in the wrong place, however unfortunately several core functions also required refactoring, so it's a bit hard to see the changes I made to your patch.</p>

      <p>Feel free to ask me about any specific changes, however these are the main flaws I noticed in your patch:</p>

      <p>1. You did not update the character db version when modifying the character table - this is reason for patch rejection as it's crucial to ensure the database is up to date before trying to use it<br />
      2. There were a lot of issues with the GetRbgBonus function which I'll outline in a moment (but thank you for documenting it with doxygen)<br />
      3. I'd consider renaming the variable in the conf file for honor points and arena points as it's not immediately clear that the value given for honor is a multiplier, whereas the value given for arena is a constant<br />
      4. (I don't really blame you for this seeing as it's not documented, but it's still an issue) Not handling the battleground list packet correctly - isRandom</p>

      <p>As for the GetRbgBonus function...</p>

      <p>1. Variable &quot;pl&quot; is poorly named - &quot;plr&quot; is the accepted shorthand for player<br />
      2. Function is poorly named - &quot;get&quot; implies a few things, most notably that<br />
      &nbsp; a. the function will not modify anything (this however is not a guarantee, use a const modifier for that)<br />
      &nbsp; b. the function will return the value you are asking for</p>

      <p>&nbsp; To implement GetRbgBonus as a &quot;Get&quot; function would mean returning some kind of array or struct, or splitting each result into its own function. I agree with your design choice to supply all variables at once and have the function modify them, however that's indicative of a &quot;Fill&quot; or &quot;Build&quot; function rather than &quot;Get&quot;<br />
      3. Using pointers where not appropriate - whenever you supply a pointer, the function using it has to verify the pointer is not nullptr, which leads to pointless boilerplate and crashes if people forget to check it. Use references instead of pointers where possible. It's also worth noting that you can't supply a reference as a default argument, which means when building the packet using only honor points for winning, honor points for losing and arena points for winning, you still have to declare a variable to store arena points for losing. I consider this to be a minor inconvenience compared to the headache of dealing with pointers.<br />
      4. Passing in an entire object where not necessary - You passed in the player object when all you needed to know was whether the player had won a battleground that day and what their level was. If you only need access to an object's data, consider just passing in the data rather than the entire object - it makes the function much easier to test and refactor.<br />
      5. Writing a function knowing that it can cause a server crash - this is never, ever okay. If a function receives bad input it should throw an exception or silently return (depending on context), not just try to use it anyway. I appreciate you documenting this, but it's still important to make sure functions fail safely.</p>

      <p>That's all I can think of off the top of my head, I'm sure I could nitpick some more but honestly, the patch was mostly fine - almost all of the issue were in a single function that was designed to be reasonably consistent with existing functions. It's just a shame the existing functions are such a mess ;)</p>

      <p>Thank you for your patch, we greatly appreciate you taking the time to contribute to AscEmu.
      </p>

      <p>&nbsp;</p>

      <p>Hey,</p>

      <p>I really appreciate detailed comment. Next time I will be more carefull while writing code :)</p>