neurolabusc / surf-ice

GLSL surface rendering source code. Compiled versions available from NITRC. Loads 3DS, CTM, DXF, FreeSurfer, GII (GIfTI), GTS, LWO2, MS3D, MZ3, NV (BrainNetViewer), OBJ, OFF, PLY, STL, VTK. Tractography formats include BFloat, PDB, TCK, TRK, and VTK. Also NIfTI format voxelwise images.
https://www.nitrc.org/plugins/mwiki/index.php/surfice:MainPage
BSD 2-Clause "Simplified" License
104 stars 23 forks source link

Add colorbarcolor to script interface. #26

Closed ningfei closed 3 years ago

ningfei commented 3 years ago

Also fix deprecated Python API functions.

neurolabusc commented 3 years ago

@ningfei I have tried to include all of your suggestions in the latest commit. Therefore, if you agree, you can close this pull request. The one issue is that the latest version of Python-for-Lazarus (which will be automatically installed with Lazarus) only supports modern Python 3. versions. Since I want to deploy my software to old systems that still have Python 2.7, I use a legacy fork of Python-for-Lazarus which supports BOTH Python 2.7 as well as more recent 3.. The code in the new version is much cleaner, but they removed the call PyString_AsDelphiString and added a call PyUnicode_AsUTF8. Therefore, it is hard to have one code base that supports everything. For the moment, you can choose between the modern Lazarus-for-Python or the legacy one by editing the opts.inc. It is a kludge.

When I get some down time, I will back port PyUnicode_AsUTF8 to my legacy fork. That will ensure the code will compile with the both the legacy and modern versions of this library.

I appreciate your changes. Tell me if you have any other changes - this is over due for a new major release.

ningfei commented 3 years ago

Sure, will close the PR. Good to know the Python version compatibility issue with the latest version of the plugin. Looking forward for the new release!

I don't have any local changes now. But when I build it I noticed something:

So just small things. There might be also something else but can't recall now. Will let you know.

ningfei commented 3 years ago

btw, since the Windows release contains python35.zip and python35.dll, do you think it also makes sense and is possible to ship a default libpython*.so or libpython*.dylib for the other two OS. Since at least on Mac, I used to have issue with the python engine, the system default one seems not compatible, Surf-Ice got closed whenever I run the python script in it.

neurolabusc commented 3 years ago

The MacOS version should work with the Python 2.7 version that comes with your Mac. You can test this with the current releases. This is one reason to use my legacy fork for Python-for-Lazarus. Apple really does not like this approach. The Apple recommended method would be to include a bespoke copy of Python in the Application Package resource bundle. The Python would have to support both x86-64 and Apple Silicon, so it would dramatically increase the size of the executable. I think the most elegant approach is to use PythonBridge instead of Python-for-Lazarus.

We do not only need to include the libpython*.so or libpython*.dylib but also all the basic python routines (in Windows these are bundled in python35.zip). I do think this is ultimately the right approach. However, it is a huge shift to something that has been working a long time. I worry that different distributions of Linux may work with different variations of lib python, etc.

If you want to investigate this and resolve this issue, I would be very grateful. However, you may invest a lot of time and find that your solution does not support a wide range of Linux platforms, or has issues being notarized by Apple.

The approach that Surfice and MRIcroGL take is identical to Blender: a native executable that allows Python scripting. Perhaps it would be useful to see how Blender solves these issues.

Please do not take the tone of my comments wrong. I would be very enthusiastic if someone wants to develop a robust solution. It is outside my expertise, and may not be straightforward.

neurolabusc commented 3 years ago

You raise a good point about the Windows version looking for files in ..\static, I think these files are provided in .\x86_64-win64, so it should look there by default. These are the CloudFlare zlib libraries that improve loading times. This can be disabled by commenting out {$DEFINE FASTGZ} in the opts.inc file, but it is not intuitive. This project has not had much interest from other developers (probably due to using Pascal), so I have not been very careful regarding documenting the build.

ningfei commented 3 years ago

Thank you for your detailed reply. Always learn a lot from discussion with you :)

I did try the latest version. It only worked smoothly on Linux. On Mac, the python engine didn't work on my side. On Windows, there's problem if I connect my laptop to a external monitor. While my old installation works (also the latest version), it didn't work if I redownload it and put it into another folder (even with the same ini configuration), the main GUI didn't show up at all. But this is another issue.

PythonBridge did look cool and elegant! As I checked, Blender ships with a python engine on all platforms: there's a python subfolder within the installation, including python executable, libs and those basic python routines. So they don't need to worry that the system python version the user has is not compatible with Blender. I agree that it would probably take quite some time to test and make it robust across platforms. Maybe we can slowly move into that direction.

For the Windows zlib issue, I think SynZip.pas needs to be updated accordingly. But I don't have a good overview on that. Maybe you can have a look if you will find the time.

I think a lot of developers especially young generations have no experience with Pascal at all, which may make it difficult for others to contribute. But this doesn't affect the fact that Surf-Ice itself is a very cool tool! :)

neurolabusc commented 3 years ago

@ningfei I now have a minimal demo of PyLazBridge, which shows how the Python interpreter can be statically embedded into an executable. This leverages @genericptr's elegant PythonBridge. This is now very easy to embed into Surfice and MRIcroGL. The bigger issue is how to create a static Python build that works across the wide range of Linux distributions. In my experience, many neuroimaging centers still rely on ancient Linux distributions like CentOS6. In the past, I have used holy build box to generate versions of dcm2niix that run across distributions.

ningfei commented 3 years ago

Hi Chris, sorry just see it. I am traveling this week. Will look into when I go back to Berlin.