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

Can't use -1 as a property index in script events? #105

Open
seanmcleod opened this issue Jul 26, 2018 · 9 comments
Open

Can't use -1 as a property index in script events? #105

seanmcleod opened this issue Jul 26, 2018 · 9 comments
Assignees

Comments

@seanmcleod
Copy link
Member

seanmcleod commented Jul 26, 2018

I have a multi-engine aircraft and I was trying to script a throttle increase for all engines in a script. So I tried the following:

    <event name="Increase Throttle">
      <description>Increase throttle to max</description>
      <condition> simulation/sim-time-sec >= 1.0 </condition>
      <set name="fcs/throttle-cmd-norm[-1]" value="1" type="FG_RAMP" tc="2" />
    </event>

However that results in an exception being thrown:

throw string("unterminated index (looking for ']')");

If I refer to all the engines individually then the script works as expected, e.g.

    <event name="Increase Throttle">
      <description>Increase throttle to max</description>
      <condition> simulation/sim-time-sec >= 1.0 </condition>
      <set name="fcs/throttle-cmd-norm[0]" value="1" type="FG_RAMP" tc="2" />
      <set name="fcs/throttle-cmd-norm[1]" value="1" type="FG_RAMP" tc="2" />
    </event>

Now FGFCS::SetThrottleCmd() has explicit code to handle an index of -1.

void FGFCS::SetThrottleCmd(int engineNum, double setting)
{
  if (engineNum < (int)ThrottlePos.size()) {
    if (engineNum < 0) {
      for (unsigned int ctr=0; ctr<ThrottleCmd.size(); ctr++)
        ThrottleCmd[ctr] = setting;
    } else {
      ThrottleCmd[engineNum] = setting;
    }

However it looks as if the parsing code used by the script while processing <set> elements doesn't support negative index values.

/**
 * Parse the optional integer index for a path component.
 *
 * Index: "[" [0-9]+ "]"
 */
static inline int
parse_index (const string &path, int &i)
{
  int index = 0;

  if (path[i] != '[')
    return 0;
  else
    i++;

  for (int max = (int)path.size(); i < max; i++) {
    if (isdigit(path[i])) {
      index = (index * 10) + (path[i] - '0');
    } else if (path[i] == ']') {
      i++;
      return index;
    } else {
      break;
    }
  }

  throw string("unterminated index (looking for ']')");
}

Is this limitation an oversight/bug or is this by design in terms of script events explicitly not supporting negative indices?

@seanmcleod
Copy link
Member Author

seanmcleod commented Jul 26, 2018

What I also noticed is that if I refer to the property without an index then the script appears to take that as an alias for the 0 index, e.g.

    <event name="Increase Throttle">
      <description>Increase throttle to max</description>
      <condition> simulation/sim-time-sec >= 1.0 </condition>
      <set name="fcs/throttle-cmd-norm" value="1" type="FG_RAMP" tc="2" />
    </event>

Based on looking at the logged values for the 2 engines, i.e. I see the thrust increasing for engine 0 but no change in thrust for the other engine.

@andgi
Copy link
Collaborator

andgi commented Jul 26, 2018

I think both issues here are by the design of the property tree. That the property fcs/throttle-cmd-norm
is the same as fcs/throttle-cmd-norm[0] certainly is - and you wouldn't want to have it differently since it'd be ugly and impractical to end the name of all single properties by "[0]".

@seanmcleod
Copy link
Member Author

@andgi so you're suggesting that you think that some-property[-1] isn't supported by the property tree by design. In which case is the -1 convention as seen and supported by functions like FGFCS::SetThrottleCmd(int index, double setting) only available to C++ code and not via script?

@andgi
Copy link
Collaborator

andgi commented Jul 26, 2018

Yes, I think so. The property tree code (while not the script interface) comes from FlightGear/SimGear and I've never ever heard about any "-1" indices. Given that the tree can be serialized into XML (also by design) indices outside 0, 1, ... would be very strange indeed. (As would non-continuous ranges of indices but I think those are supported, though.)

Is it a very big problem to repeat the set-element for each engine?

Of course you could add a special case in the script parser fcs/throttle-cmd-norm[-1] but then someone else wants it for some other engine properties, and for all gear units and....

@seanmcleod
Copy link
Member Author

I had come across the -1 option for FGFCS::SetThrottleCmd() a couple of years ago while developing a simulator for test pilot training where I needed to support single and multi-engine models using a single physical throttle that I was interfacing with and so used the -1 option for convenience.

Then more recently I noticed a bug in the engine initialization code for handling <running> -1 </running> elements, see - #101.

So I assumed that there was general support for the -1 index option 😉

In terms of whether it's a big problem to add repeated <set> elements the main issue is I'm trying to write a script, in this case an acceleration test, that is agnostic to how many engines there are, i.e. ideally I'd like to be able to invoke the script for any aircraft independent of exactly how many engines it has.

IF we decided to add more support for the -1 index I wouldn't special case fcs/throttle-cmd-norm.

My thinking is that the script code would detect the -1 index and in that case internally it would query the property manager to figure out exactly how many indices have been created for that property and it would then generate multiple <set> elements internally to match the number of actual indices.

That way script writers could write scripts that are agnostic to the exact number of indices for properties that they want to set uniformly for a range of indices, e.g. across engines etc.

@andgi
Copy link
Collaborator

andgi commented Jul 27, 2018

@seanmcleod Neither of those examples are directly indexing properties in the property tree.

@seanmcleod
Copy link
Member Author

Correct, but they led me to believe as a user that there was a good chance that the some-property[-1] case in a script would also be supported given that the C++ API FGFCS::SetThrottleCmd() supported it and the initialization file also supported the use of a -1 index to allow an operation across all indices.

And I think it would be a useful feature for users writing scripts, particularly as I mentioned for writing scripts that want to work across aircraft with varying numbers of engines as one example, and any other aircraft properties exposed via indices.

@bcoconni
Copy link
Member

@seanmcleod
What is the way forward about this issue ?

@seanmcleod seanmcleod self-assigned this Mar 18, 2019
@seanmcleod
Copy link
Member Author

@bcoconni I've assigned it to myself in the meantime. I had started writing some code at the time to test out my proposal of having the script code generate multiple set entries for any that use a -1 index, but then got distracted with other things. I'll have to revisit the code.

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

No branches or pull requests

3 participants