zanoni-mbdyn / blendyn

MBDyn (https://www.mbdyn.org/) graphical post-processor for blender (https://www.blender.org/)
GNU General Public License v2.0
40 stars 8 forks source link

Fix simtime and remove SimulationStatus #20

Closed janga1997 closed 7 years ago

janga1997 commented 7 years ago

I've fixed the current simulation time bug that @louisgag pointed out, and removed the Check Simulation Status button, which is of no use as the simulation status is displayed in real time.

janga1997 commented 7 years ago

I felt like Blendyn needed a .gitignore, as I always had to be careful not to commit changes in tests/ and __pycache__

janga1997 commented 7 years ago

I found out about this too late, but the shutil module in the Python standard library can handle everything that we are trying to do with setting the Installation path of mbdyn.

shutil.which('mbdyn') returns /usr/local/mbdyn/bin/mbdyn, similar to which mbdyn in the bash shell.

zanoni-mbdyn commented 7 years ago

Cool. Do you want to have a go at using shutil instead of asking the user to manually set the path?

janga1997 commented 7 years ago

Yup. That would reduce some steps for the user, and make our code more simpler.

janga1997 commented 7 years ago

There's just one kicker. By default, shutil.which(), like the unix which command, looks for all bin/ directories in the system $PATH variable.

If the user installs mbdyn to a local directory, and doesn't add it to the system PATH variable, it wouldn't work in the terminal, and it wouldn't work in Blender.

In short, he has to make it sure it works outside of Blender before trying it in Blender. (It doesn't seem a hassle to me)

louisgag commented 7 years ago

If the user installs mbdyn to a local directory, and doesn't add it to the system PATH variable, it wouldn't work in the terminal, and it wouldn't work in Blender.

yes, I think it is important to leave the manual option (perhaps fill in the default values with which). I don't not always use the MBDyn of my $PATH as I have different versions running.

janga1997 commented 7 years ago

I've added the previous iteration of setting installation path again, but it isn't compulsory.

If the user doesn't set it (which means no config.json in the addons directory), we just the default mbdyn path returned by shutil.which() If the user just wants to use the default one , and not the one he may have created in the previous simulations , he can just click on the Use Default MBdyn option.

that should cover it?

louisgag commented 7 years ago

I'd say that's a good approach indeed

zanoni-mbdyn commented 7 years ago

I like the approach, but I'd suggest a small modification:

The Use Default MBDyn button can be converted to an Operator, that tries to set tha PATH using shutil.which(). It is useful if the user wants to quickly revert to the default.

As a side note: a header should be also added to the config.json file, that should be placed in the blendyn directory and not just in the addon folder of Blender.

janga1997 commented 7 years ago

I am not sure how making Use Default MBDyn into an operator would be any different than just a checkbox, because even in the operator we would be switching the bool value of mbs.default_mbdyn.

janga1997 commented 7 years ago

And to make sure about the last point, you want the config.json file to be in the blendyn/ directory, and not in the addons/ directory.

zanoni-mbdyn commented 7 years ago

Yes, please put the config.json into the Blendyn directory (use the mbs.addon_path property of the MBDynSettingsScene collection).

The way I see the UI for the MBDyn PATH work is this: when a config.json or the value returned from shutil.which() method are available, show the MBDyn PATH in the UI (i.e., set the value of the mbs.install_path property so that it is shown in the panel). When it is not, then do not show anything or, better still, set the default value of intall_path to "not found" or something similar.

Under the PATH field, I'd like to have two buttons: one is the already available "Set Installation Path". The other one is "Use Default MBDyn", that tries to set the install_path property using shutil.which().

Just get rid of the mbs.defaul_mbdyn property, it is useless with this approach.

zanoni-mbdyn commented 7 years ago

Also, testing this PR I came across this error, while closing Blender:

Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/home/zanoni/.config/blender/2.78/scripts/addons/blendyn-master/baselib.py", line 721, in delete_log
    os.remove(logFile)
NameError: name 'logFile' is not defined
janga1997 commented 7 years ago

I had noticed something like that myself, but haven't been able to pinpoint it.

did the error arose from an unsaved blend file, or a saved one?

zanoni-mbdyn commented 7 years ago

An unsaved one

janga1997 commented 7 years ago

I'm a little skeptical on the approach for installation path you laid out above.

If the variable mbs.install_path is used to contain both the default MBDyn path, and the path from the config.json file, the user risks overwriting the config.json file with the default MBDyn path, by pressing on the Set Installation Path button after clicking on the Use Default MBDyn.

Using a bool value to switch between default and custom MBDyn paths prevents this.

zanoni-mbdyn commented 7 years ago

Well what I had in mind was to check the mbs.intall_path value when drawing the panel: if it is still on the default value, then we try to set it with shutil.which(). If it is not, we don't do anything.

I'd reserve the Set Installation Path operator for the manual selection of the MBDyn PATH, no automatic stuff there.

The automatic stuff can be called manually with the Use Default MBDyn button, in my ideal panel.

But thinking about that, maybe it is an unnecessary overkill to check continuously the value of mbs.intall_path. We could skip that part and leave it to the user to press the Use Default MBDyn button the first time.

janga1997 commented 7 years ago

As I am working on this, i am confused about the role of the Set Installation Path operator.

Does it set the value of mbs.install_path from the config.json file, or does it set the value in the config.json file? I seem to understand from your comments that it sets from the file, in that case, we would have to have a seperate operator to change/set the value in config.json .

zanoni-mbdyn commented 7 years ago

@janga1997 the Set Installation Path sets the value in config.json. Actually, the config.json file right now was completely useless... So I've added an app handler that loads it at startup, or alternatively tries to use shutil.which().

Please pull the commit from the janga1997-fix-run-mbdyn branch.

I think that you can figure out how to adjust the rest now.

janga1997 commented 7 years ago

I had to add another operator to switch to the path set in the config.json file.

So here is how it works now,

  1. We try to load the install_path from either the config.json file, or shutil.which at startup.

  2. If both fail, install_path will still show not yet loaded, which is the default value.

  3. The user can set mbs.install_path to shutil.which by pressing Use Default MBDyn

  4. The user can set mbs.install_path to the value in config.json by pressing Use Config MBDyn (These two buttons are required to switch back and forth easily)

  5. The user can change the value in config.json by pressing Set Installation Path as before.

zanoni-mbdyn commented 7 years ago

Merged.