Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#350 TF2 positional audio plugin

closed
nobody
None
5
2012-10-30
2011-08-20
SizzlingCalamari
No

Updated to support build 4659 on August 18th, 2011.

Discussion

  • Stefan H.
    Stefan H.
    2011-08-20

    Hi. Thanks for the patch. C-Programmer? ;-) Haven't found the time to verify functionality yet, here my comments on the patch itself:

    If the steamcommunity link in the copyright notice is really the best way to contact you then that's fine, any reason not to use a plain email (e.g. the sourceforge one) though?

    The reformat of the BSD licensing terms just clutters the patch and should be removed.

    Same for the memloc explanations, though their formatting is kinda screwed to begin with ;-) . In any case the explanation of the memory locations should be placed closer to the #define's (or maybe also use the defines names in the first place. That would at last end the "oops I didn't update the comments" issue...) now that there's only one place they pop up.

    The changes to calcout are fine with me though I don't really think one line - multiple variable definitions are very readable (and while I'm nitpickping the float cosh, cosv etc. could be const).

    fetch is a bit strange. Why remove the "always clear the variables first" at the top. It was put there to prevent from screwing up later:
    + if (teamid == 0 || teamid == 1)
    return true; // Deactivate plugin
    For example relies on those variables (well not all but some of them) being zero or the plugin won't deactivate. On the other hand if peek proc fails and false is returned setting those variables to 0 doesn't have any effect since the plugin will get detached and the values ignore anyways.

    Regarding szHost: Does the new memory location no longer need the special handling for connecting to the default port (before the location could contain the addr with and without port in that case which had to be regarded).

    Also you seem to introduce functional changes to the way we fill the context. The previous code was outputting json format while the new one seems to be some csv. Json format was chosen to make it easy for serverside scripts to use the data wile keeping us able to expand the plugins reporting capabilities later on (see bf2 plugin). Also while sprintf definitely is faster in this case we choose the ostringstream to be able to assemble the string as we go without having to bother reserving memory beforehand. This should definitely be kept in sync throughout the plugins and we are willing to take the (probably minor) performance hit for that.

    trylock contains an unchecked peekProc.

    That's about it ;-) Thanks again for your work.

     
  • Fixed all the stuff you talked about. Thanks

     
  • Stefan H.
    Stefan H.
    2011-08-20

    Great. Looks much better.

    You still had different formatting for the bsd header. Since you patched your old patch I squashed both and while at it fixed the formatting (also did some minor modifications).

    Since sf won't let me append anything to this ticket you can find the updated patch here (if you find anything wrong with it or I should split my modifications out of there let me know):
    http://d0t.dbclan.de/dump/mumble_patches/0001-Updated-TF2-positional-audio-plugin-to-support-build.patch

    One question though. The user.email / user.name you set in your git (and hence are in the patch header) differ from what's in the copyright notice. Which one of those do you prefer?

    Unless you have any objections I (or one of our other core devs) will apply the patch once we tested it (and the naming issue is resolved ofc).

     
  • Mr. Rawnar
    Mr. Rawnar
    2011-08-21

    In calcout() you first calculate the results of sin() and cos() and fill some variables with the results. The idea behind it is that you do not need to call cos and sin twice with the same argument. But according to slicer you do not need to do this implicitly as the compiler will do this for you. I did the same in a previous attempt, but slicer seems to like the old way. So just leave the calls to sin() and cos() as they were and let the compiler optimize it.

    Oh and is there a reason why you have expanded all the for loops. In general slicer likes to keep the amount of line changes to the minimal, so other devs will have less problems applying there patches when they are hacking on the same code.

     
  • Stefan H.
    Stefan H.
    2011-08-21

    @sin/cos: The compiler does indeed optimize that (verified it for vc10 and gcc4.6) so we should keep the old code for that.

    I personally don't care that much about what happens with the loops. (3 line loop vs 3 assignments, the compiler will unroll that anyway I guess....).

     
  • It's fine however you choose to change it. If the results are the same, the more readable version would be the way to go. Thanks.

     
  • Kissaki
    Kissaki
    2012-10-27

    Thanks for your patch!

    Valve released an update to TF2 today though that features native positional audio for Mumble (via the link plugin).
    That means we no longer have to manually scan for location data, nor do we have to update our plugin after each patch.