Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Console Function Calls #81

Closed
wants to merge 5 commits into from

Conversation

jamesu
Copy link
Contributor

@jamesu jamesu commented Oct 12, 2012

This branch makes the following changes to the Console/TorqueScript system:

  1. Changes the Console Function & Method binding code so values are passed using a native console value wrapper (ConsoleValueRef) instead of just strings.
  2. Optimizes the case of "%var = %othervar".
  3. Changes the interpreter and compiler to support no.1 and to reduce unneccesary type conversion when calling functions.

In essence, this improves the performance of TorqueScript so that calling functions is no longer expensive in the case of passing numbers around. It also optimizes variable assignment, so when assigning numeric variables it will no longer perform an unneccesary string conversion.

Relevant benchmarking figures are available here:

http://www.garagegames.com/community/forums/viewthread/131792/4#comment-834494
http://www.garagegames.com/community/forums/viewthread/131792/5#comment-834766

The actual benchmarking code I used is here: https://gist.github.com/3760645

While I have a few more ideas to improve the scripting engine (e.g. improving field access), I feel that changing the way script functions are binded to the engine is both significant and useful enough to make a pull request now.

This code has been tested on Windows and Linux. It has also been tested in the case of Singleplayer and Multiplayer. Unfortunately while I have tested this as much as I can, there could still be edge cases where this breaks something so I encourage anyone interested in incorporating these changes to test them against their script code.

…le code.

- ConsoleValue class is now the base value class.
- ConsoleValueRef is now used to supply function parameters. Values are disposable.
- Script functions return values instead of just strings where possible.
- Variables can be disposable strings
- Bytecode changed

Fix the issues with console method parameters and fields which prevented missions from loading.
- Prevent stack corruption in a few places
- Use correct type in printfs
- Reduce type conversions in EngineApi & dAto*
- Fix compilation on GCC
- Tidy up code
@DavidWyand-GG
Copy link
Member

Hey James.

Before we can accept any changes to the repositories we'll need you to sign the Open Source Software Agreement. We've outlined the steps in the wiki: Contributing Page Please follow those steps and let me know when you're done. Thanks!

@jamesu
Copy link
Contributor Author

jamesu commented Nov 9, 2012

Pushed another commit to fix the ambiguity between signed and unsigned integers.

Also signed the agreement.

@DavidWyand-GG
Copy link
Member

@jamesu does this pull request contain all of your changes? I would like to start looking at what you've done.

@jamesu
Copy link
Contributor Author

jamesu commented Nov 23, 2012

@DavidWyand-GG In terms of the function calls, consolefuncrefactor is the one you want. This deals with function calls, and is fairly stable (AFAIK).

The other branch in my repository (consoletyperefactor) deals with changing the type system. Unfortunately there are a few killer bugs in the editors in that branch, so it isn't suitable for inclusion at this time.

@DavidWyand-GG
Copy link
Member

These changes have been placed into a consolefuncrefactor branch on the main repo and the community has been asked to help test. We'll let the testing happen over the next couple of weeks, and then integrate these changes with the development branch if all goes well.

@crabmusket
Copy link
Contributor

I have been using this while developing tasman and have come across one difference to stock T3D so far involving comparisons with floating-point numbers. However this is already completely dodgy in scripts anyway. For example:

$m = 2/3;
$m == 2/3;
>> 0

So on one hand I wouldn't worry too much about it. On the other, if someone is relying on the current badly-defined behavior, they may get a surprise.

I discovered this some time ago and managed to reproduce it in the console, but I've forgotten the sequence that demonstrated it. At the moment, all I can say is this tasman test fails in the refactored branch, but not in stock:

expect(2 / (2+1)).toEqual(2/3);

With the error:

expected 0.666667 to equal 0.666667

The reason I can't demonstrate this simply is because tasman internally shifts values around quite a bit to give a nice interface. So diagnosing the exact problem may take me some time.

Don't ask my why I tested that! I was just messing around. But it seems to have found something after all...

@crabmusket crabmusket added this to the 3.7 milestone Mar 28, 2014
@jamesu
Copy link
Contributor Author

jamesu commented May 18, 2014

If I recall correctly in this branch type of variables will be maintained when you pass them through functions rather than being converted to strings.

It could be that the float is being converted to a string which will print out the number in the same precision for both values, despite the comparison normally failing. Floating point numbers can be very strange!

@LuisAntonRebollo
Copy link
Contributor

Can one of the admins verify this patch?

@jamesu
Copy link
Contributor Author

jamesu commented May 30, 2014

@LuisAntonRebollo This has been stagnating for over a year, I'm not holding out much hope for it atm ;)

@jamesu
Copy link
Contributor Author

jamesu commented Sep 25, 2014

Since there is clearly no interest in attempting to merge this in, I will be removing this as a pull request. Rights to use the code as per the contribution agreement remain as it has been incorporated into the consolefuncrefactor branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants