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

The python example has fdm.FGFDMExec twice, with different explanations #798

Closed
1 of 3 tasks
Pomax opened this issue Dec 29, 2022 · 21 comments · Fixed by #820
Closed
1 of 3 tasks

The python example has fdm.FGFDMExec twice, with different explanations #798

Pomax opened this issue Dec 29, 2022 · 21 comments · Fixed by #820
Assignees

Comments

@Pomax
Copy link

Pomax commented Dec 29, 2022

I'm submitting a ...

  • bug report
  • feature request
  • support request => Please do not submit support request here, see note at the top of this template.

Describe the issue

The examples/python/PlotAoaVsCAS.py script contains the following contradictory code:

# Path to JSBSim files
PATH_TO_JSBSIM_FILES="../../"

fdm = jsbsim.FGFDMExec(PATH_TO_JSBSIM_FILES) 

and

    # Points to jsbsim executable and make an object
    fdm = jsbsim.FGFDMExec(PATH_TO_JSBSIM_FILES) 

Either jsbsim.FGFDMExec needs the path for the jsbsim directory, in which case the second code block is incorrect, or it needs the path for the jsbsim executable, in which case the "constant" in the first block is wrong.

What is the current behavior?

We can't run this example =)

What is the motivation / use case for changing the behavior?

Projects live or die based on having good docs and working examples =)

Other information

It's also curious that jsbsim.FGFDMExec gets run three times, does it need to be recreated every iteration? One would assume that once fdm has been set up, one simply keeps using that.

@Pomax
Copy link
Author

Pomax commented Dec 29, 2022

As an additional bug, the example tries to load global5000/global5000.xml from the aircraft directory, and while that may have been a model in the past: in the current release, that aircraft does not exist.

@Pomax
Copy link
Author

Pomax commented Dec 29, 2022

And then as third bug, even though we won't be modifying the xml data, JSBsim tries to load the file with write permissions, which can cause problems on a properly set up Windows (running as a normal user rather than as an admin) with JSBsim installed under the standard Program Files or Program Files (x86) master directories.

Only admins have write access to those directories, so if a normal user runs the example script, it will error out with a PermissionError:

Traceback (most recent call last):
  File "C:\Users\Pomax\Documents\Git\projects\are-we-flying\api\test-jsbsim.py", line 90, in <module>
    cg = changeCG(fdm, cgPos[j], False)
  File "C:\Users\Pomax\Documents\Git\projects\are-we-flying\api\test-jsbsim.py", line 50, in changeCG
    tree.write(os.path.join(fdm.get_root_dir(), AIRCRAFT_XML))
  File "C:\Program Files\Python39\lib\xml\etree\ElementTree.py", line 732, in write
    with _get_writer(file_or_filename, enc_lower) as write:
  File "C:\Program Files\Python39\lib\contextlib.py", line 119, in __enter__
    return next(self.gen)
  File "C:\Program Files\Python39\lib\xml\etree\ElementTree.py", line 768, in _get_writer
    file = open(file_or_filename, "w", encoding=encoding,
PermissionError: [Errno 13] Permission denied: 'C:/Program Files/JSBsim\\aircraft/fokker100/fokker100.xml'

(The installer-default C:\JSBsim was common practice in the XP days, but Windows conventions have for many years now been to only install into either the system level Program Files or Program Files (x86) dirs, requiring admin rights, or to the user's own %LOCALAPPDATA%/ %ROAMINGAPPDATA% directories, which users have full read/write/execute rights for)

@Pomax
Copy link
Author

Pomax commented Dec 29, 2022

With JSBSim relocated to a throw-away location, and the aircraft model updated to the A320, the script runs to completion, although plt.show() does not appear to show anything (the script simply exits at completion).

There are also a fair number of stderr entries, which might just be because the example was intended for a completely different aircraft, but I'll attach them here anyway:

stderr.txt
stdout.txt

@bcoconni
Copy link
Member

Well, an example is an example no more, no less.

It is not meant to be successfully executed on every possible configurations that exist under the sun. More specifically, the location of the flight model data may vary from one installation to the other so, yes the example uses a constant PATH_TO_JSBSIM_FILES that is meant to be updated depending on the user configuration.

PATH_TO_JSBSIM_FILES is the path to the folder that contains the flight model data files (i.e. XML files). In most cases, it is the folder just above aircraft/, engines/ and systems.

Granted that global5000/global5000.xml does not exist, this one can be fixed easily by picking any other aircraft from the aircraft folder.

The example uses an algorithm that modifies the aircraft XML file to update the aircraft CG, so indeed it needs write access and it fails if you try to access data that are read only. But I would not go as far as calling that a bug.

@bcoconni
Copy link
Member

The installer-default C:\JSBsim was common practice in the XP days, but Windows conventions have for many years now been to only install into either the system level Program Files or Program Files (x86) dirs, requiring admin rights, or to the user's own %LOCALAPPDATA%/ %ROAMINGAPPDATA% directories, which users have full read/write/execute rights for

This one can easily be fixed.

@bcoconni
Copy link
Member

With JSBSim relocated to a throw-away location, and the aircraft model updated to the A320, the script runs to completion, although plt.show() does not appear to show anything (the script simply exits at completion).

I just ran the example on my side with the following changes and a window opened showing the plot below. Is matplotlib correctly installed ?

diff --git a/examples/python/PlotAoaVsCAS.py b/examples/python/PlotAoaVsCAS.py
index 551c554a..8ac80194 100644
--- a/examples/python/PlotAoaVsCAS.py
+++ b/examples/python/PlotAoaVsCAS.py
@@ -23,14 +23,14 @@ import os
 # Change the directory to the aircraft to be studied
 # Note - Moments of inertia are not updated
 def changeCG(fdm, cgPos, readOnly):
-    tree = ET.parse(os.path.join(fdm.get_root_dir(), 'aircraft/global5000/global5000.xml'))
+    tree = ET.parse(os.path.join(fdm.get_root_dir(), 'aircraft/A320/A320.xml'))
     root = tree.getroot()

     for x in root.findall('mass_balance/location'):
         cg = x.find('x').text
         if not readOnly:
              x.find('x').text=str(cgPos)
-             tree.write(os.path.join(fdm.get_root_dir(), 'aircraft/global5000/global5000.xml'))
+             tree.write(os.path.join(fdm.get_root_dir(), 'aircraft/A320/A320.xml'))
     return cg

 # Fuel max for Global5000
@@ -47,7 +47,7 @@ fuel=[1000,fuelmax/2,fuelmax]
 weight=["light","mid","heavy"]

 # Path to JSBSim files
-PATH_TO_JSBSIM_FILES="../../"
+PATH_TO_JSBSIM_FILES="../../.."

 fdm = jsbsim.FGFDMExec(PATH_TO_JSBSIM_FILES)
 # Get the original CG from aircraft xml

image

@Pomax
Copy link
Author

Pomax commented Dec 30, 2022

It is not meant to be successfully executed on every possible configurations that exist under the sun. More specifically, the location of the flight model data may vary from one installation to the other so, yes the example uses a constant PATH_TO_JSBSIM_FILES that is meant to be updated depending on the user configuration.

That's fair, but the original issue is that one set of code lines tells us that PATH_TO_JSBSIM_FILES represents the path for the dir, whereas the other set of lines explicitly claims it should be the path for the jsbsim executable, so based on your explanation (and the name of the var of course =) it sounds like that second instance should have the following code comment instead:

    # Point to the jsbsim directory and make an object
    fdm = jsbsim.FGFDMExec(PATH_TO_JSBSIM_FILES) 

Matplotlob related, it turns out that matplotlib itself was installed correctly, but python had been installed without the TK/IDLE extensions, so matplotlib couldn't draw anything. I updated the python install to include those, and a simple tk example works, as do the examples from the matplotlib website. Rerunning the script with your diff now generates a graph, although it looks very different from yours?

image

@seanmcleod
Copy link
Member

seanmcleod commented Dec 31, 2022

As an additional bug, the example tries to load global5000/global5000.xml from the aircraft directory, and while that may have been a model in the past: in the current release, that aircraft does not exist.

Hmm, the aircraft global5000 is in the repo, I even edited it on Jan 8th 2022 and it was added to the repo on May 28th 2021.

https://github.com/JSBSim-Team/jsbsim/tree/master/aircraft/global5000

So either you have an old installer that didn't include the aircraft but included the example script, or somehow the installer isn't including this particular aircraft?

@seanmcleod
Copy link
Member

So I've just run PlotAoaVsCAS.py from my JSBSim repo without making any changes, i.e. the script is using the global5000 aircraft that exists in the repo.

Global5000

image

Now I noticed in the diff that @bcoconni posted above that he changed 2 references to global5000 to A320, but there is another reference that he missed.

fdm.load_model('global5000')

Which means using @bcoconni's diff he's really making use of the global5000 aircraft which he must have in his repo, and the cg changes are being applied to the A320 model, i.e. they're having no effect. So by making the same changes as he did I get the same result.

Global5000, with cg changes to A320

image

So @Pomax I'm guessing you must have either changed all three references to A320 or to some other aircraft like fokker100 which is why you don't see the same graph output as @bcoconni.

@bcoconni
Copy link
Member

As an additional bug, the example tries to load global5000/global5000.xml from the aircraft directory, and while that may have been a model in the past: in the current release, that aircraft does not exist.

Hmm, the aircraft global5000 is in the repo, I even edited it on Jan 8th 2022 and it was added to the repo on May 28th 2021.

https://github.com/JSBSim-Team/jsbsim/tree/master/aircraft/global5000

So either you have an old installer that didn't include the aircraft but included the example script, or somehow the installer isn't including this particular aircraft?

Oops ! I was so convinced that the global5000 was not part of the repo that I didn't bother to check 😞

@bcoconni
Copy link
Member

Now I noticed in the diff that @bcoconni posted above that he changed 2 references to global5000 to A320, but there is another reference that he missed.

fdm.load_model('global5000')

Well, hum, it seems I reviewed this issue too hastily.
Thanks for correcting my mistakes @seanmcleod 👍

@Pomax
Copy link
Author

Pomax commented Jan 1, 2023

So I've just run PlotAoaVsCAS.py from my JSBSim repo without making any changes, i.e. the script is using the global5000 aircraft that exists in the repo.

I installed using JSBSim-1.1.13-setup.exe from https://github.com/JSBSim-Team/jsbsim/releases and my aircraft dir does not contain that aircraft:

d:\JSBSim\aircraft>dir /b
737
A320
A4
ah1s
aircraft_template.xml
B17
B747
ball
blank
Boeing314
C130
c172p
c172r
c172x
c182
c310
Camel
Concorde
DHC6
dr1
f104
f15
f16
f22
F4N
F80C
fokker100
fokker50
J246
J3Cub
L17
L410
MD11
minisgs
mk82
OV10
p51d
pa28
paraglider
pc7
pogo-jsbsim
Pterosaur
SGS
sgs126
sgs233
Short_S23
Shuttle
Submarine_Scout
T37
T38
t6texan2
weather-balloon
wrightFlyer1903
X15
x24b
XB-70
ZLT-NT

Does the release maybe forget to bundle it in? Comparing the release and the repo, there's a surprising mismatch between the aircraft directories...

@seanmcleod
Copy link
Member

Hmm, we'll need to take a look at the scripts that generate the builds to see why some aircraft are being left out. Doing a quick eyeball I see the following missing from your list compared to what is in the repo.

Missing:
787-8
F450
global5000
LM

@seanmcleod
Copy link
Member

So taking a quick look it looks like this is the file that is used by the setup program to generate the Windows installer.

https://github.com/JSBSim-Team/jsbsim/blob/master/JSBSim.iss.in

And in particular in terms of including the aircraft:

Source: "${PROJECT_SOURCE_DIR}\aircraft\*.xml"; DestDir: "{app}\aircraft"; Flags: recursesubdirs; Components: data

At first glance there is nothing obvious from the setup program's input file that would indicate why there are a handful of missing aircraft directories.

Given the search pattern ${PROJECT_SOURCE_DIR}\aircraft\*.xml it looks like a handful of files will be excluded from aircraft directories, e.g. INSTALL, Authors.txt, README, but this doesn't explain the completely missing aircraft you're seeing.

(base) C:\source\jsbsim>dir .\aircraft\737\


    Directory: C:\source\jsbsim\aircraft\737


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a----         1/31/2022  11:01 PM          35126 737.xml
-a----          1/1/2023   3:46 PM            504 cruise_init.xml
-a----         2/27/2018  10:01 PM            485 cruise_steady_turn_init.xml
-a----         2/27/2018  10:01 PM            414 INSTALL
-a----         2/27/2018  10:01 PM            583 reset00.xml


(base) C:\source\jsbsim>dir .\aircraft\787-8\


    Directory: C:\source\jsbsim\aircraft\787-8


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a----          1/1/2023   3:33 PM          30787 787-8.xml
-a----          6/4/2022   9:13 PM            976 Authors.txt
-a----          6/4/2022   9:13 PM            375 README


(base) C:\source\jsbsim>dir .\aircraft\global5000\


    Directory: C:\source\jsbsim\aircraft\global5000


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----         6/30/2021   6:17 PM                scripts
-a----         6/30/2021   6:17 PM            457 airborne.xml
-a----        12/31/2022   2:49 PM          27237 global5000.xml
-a----         6/30/2021   6:17 PM          11649 global5000ap.xml
-a----         6/30/2021   6:17 PM           7699 global5000apx.xml


(base) C:\source\jsbsim>dir .\aircraft\global5000\scripts\


    Directory: C:\source\jsbsim\aircraft\global5000\scripts


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
-a----         6/30/2021   6:17 PM           2404 trim-cruise.xml
-a----         6/30/2021   6:17 PM           3181 trim-cruise_ap.xml


(base) C:\source\jsbsim>

@Pomax
Copy link
Author

Pomax commented Jan 1, 2023

If it helps: the source archive over on https://github.com/JSBSim-Team/jsbsim/archive/refs/tags/v1.1.13.zip also doesn't contain those aircraft, so this might not be windows-specific.

@bcoconni
Copy link
Member

bcoconni commented Jan 1, 2023

Got it ! They are missing from the branch release/v1.1 which the releases are based on.

@seanmcleod
Copy link
Member

@Pomax fyi I've updated some of the comments in PlotAoaVsCAS.py and pushed it to master.

Also if you're interested in another Python example I'm in the process of adding another one, see #802.

@Pomax
Copy link
Author

Pomax commented Jan 2, 2023

Nice! More examples are always good.

For PlotAoaVsCAS, is the fdm = ... still needed on line 66? Does the instance on L52 need to be overwritten at each iteration? (if so, it might be good to put in a different code comment that explains why that new instance is needed as opposed to having a reset() call to clear everything, if that's what's happening of course)

@bcoconni
Copy link
Member

bcoconni commented Jan 28, 2023

The installer-default C:\JSBsim was common practice in the XP days, but Windows conventions have for many years now been to only install into either the system level Program Files or Program Files (x86) dirs, requiring admin rights, or to the user's own %LOCALAPPDATA%/ %ROAMINGAPPDATA% directories, which users have full read/write/execute rights for

This one can easily be fixed.

This is now fixed in the master branch (commit 0880dbe)

@bcoconni
Copy link
Member

I have also submitted the PR #820 that groups global variables in the first lines so that a user would know what are the parameters that they should modify to suit their need.

Once the PR #820 will be merged, this issue will be closed.

@Pomax
Copy link
Author

Pomax commented Jan 28, 2023

awesome, thank you so much!

bcoconni added a commit to bcoconni/jsbsim that referenced this issue Apr 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants