Studio python module by glebmish · Pull Request #1464 · appleseedhq/appleseed

glebmish · 2017-06-17T14:50:51Z

Created new Python module with appleseed.studio specific binding such as:

open_project: opens project in UI by its path
save_project: saves project that is currently opened in UI
new_project: creates new project and shows it in UI
current_project: returns currently opened project as Project object
is_project_dirty: returns True if project is changed after last save
close_project: closes project and cleans UI

est77 · 2017-06-17T14:57:51Z

src/appleseed.studio/mainwindow/mainwindow.cpp


+void MainWindow::new_project()
+{
+    if (!can_close_project())


I think that in scripting, we always want to close existing project, even if it has unsaved changes.
We should move the can_close_project() test back to the slot.
What do you think?

agree
but maybe we don't want to close project if it's unsaved? Maybe it would be a good idea to introduce force flag here? And if force==false and project is not saved then throw exception

yes, good idea.

est77 · 2017-06-17T15:00:15Z

src/appleseed.studio/mainwindow/mainwindow.cpp


 void MainWindow::build_python_console_panel()
 {
+    PythonInterpreter::instance().set_mainwindow(this);


Can we move this to the place where the mainwindow is created?
It's not really related to the python console panel.

est77 · 2017-06-17T15:01:16Z

src/appleseed.studio/python/pythoninterpreter.h

 // appleseed.foundation headers.
 #include "foundation/core/concepts/noncopyable.h"

+#include "mainwindow/mainwindow.h"


Use a forward declaration.

that way I need to include mainwindow.h in module.cpp and in every file where I use PythonInterpreter and get MainWindow from it. Is it ok?

Yes, it is ok. It's more work, but it's better and we already do it everywhere in the rest of the code.

dictoon · 2017-06-18T16:01:26Z

src/appleseed.studio/mainwindow/mainwindow.cpp

 #include <algorithm>
 #include <cassert>

+#include "python/pythoninterpreter.h"


Move to the appleseed.studio headers section.

dictoon · 2017-06-18T16:03:14Z

src/appleseed.studio/mainwindow/mainwindow.cpp

    m_project_manager.load_project(filepath.toAscii().constData());
 }

+void MainWindow::new_project()


Maybe move before open_project() to follow the natural progression: new, open, save, save as, etc.

dictoon · 2017-06-18T16:04:23Z

src/appleseed.studio/python/pythoninterpreter.cpp

@@ -85,7 +98,9 @@ PythonInterpreter::PythonInterpreter()
    // Imports appleseed module and creates asd alias for it


Update comment now that there are two modules.

dictoon · 2017-06-18T16:04:40Z

src/appleseed.studio/python/pythoninterpreter.cpp

@@ -85,7 +98,9 @@ PythonInterpreter::PythonInterpreter()
    // Imports appleseed module and creates asd alias for it
    // so we can refer to it by both names.
    PyRun_SimpleString("import appleseed\n"


All imports first, all namespace aliases next.

dictoon · 2017-06-18T16:05:32Z

src/appleseed.studio/python/module.cpp

+    studio::PythonInterpreter::instance().get_mainwindow()->open_project(project_path);
+}
+
+void new_project()


Reorder here too: new_project() first, open_project() second

dictoon · 2017-06-18T16:07:43Z

src/appleseed.studio/python/pythoninterpreter.h

    void execute_command(const char* command);

    void redirect_output(OutputRedirector redirector);
+    void set_mainwindow(MainWindow* mainWindow);


I'd suggest reordering methods of this class to follow a more logical progression:

instance set_mainwindow get_mainwindow redirect_output execute_command

dictoon · 2017-06-18T16:07:59Z

src/appleseed.studio/python/pythoninterpreter.h


    void redirect_output(OutputRedirector redirector);
+    void set_mainwindow(MainWindow* mainWindow);
+    MainWindow* get_mainwindow();


get_mainwindow() should be made const.

I can't because then I can't use its non-const functions
const MainWindow* PythonInterpreter::get_mainwindow()
{
return mainWindow;
}

PythonInterpreter::instance().get_mainwindow().->open_project();

I'm not talking about returning a const MainWindow*, I'm just saying you should make get_mainwindow() itself const, which isn't the same thing:

MainWindow* get_mainwindow() const;

dictoon · 2017-06-18T16:12:29Z

src/appleseed.studio/python/module.cpp

+
+#include "foundation/platform/python.h"
+
+#include "pythoninterpreter.h"


Headers should be ordered from the most specific first (appleseed.studio headers first in this case) to the most general (typically standard headers). This order was chosen because it helps catching missing header files. Several explanations and details can be found here: https://stackoverflow.com/questions/2762568/c-c-include-file-order-best-practices

Also, add the section comments: "appleseed.studio headers", "appleseed.foundation headers", etc.

dictoon · 2017-06-18T16:14:07Z

src/appleseed.studio/python/pythoninterpreter.cpp

 namespace bpy = boost::python;
 namespace bf = boost::filesystem;

+extern "C" void init_appleseedstudio();


Where is this function defined?

it is generated by BOOST_PYTHON_MODULE(_appleseedstudio) in module.cpp

glebmish · 2017-06-21T07:21:26Z

src/appleseed.studio/mainwindow/mainwindow.cpp

+    m_project_manager.load_project(filepath.toAscii().constData());
+}
+
 void MainWindow::open_and_render_project(const QString& filepath, const QString& configuration)


idk if I should expose this

for now lets not expose it

I thought you wanted to remove MainWindow::open_and_render_project()?

no, it was there before. Should I?

Ah ok, my bad :) It's fine then.

glebmish · 2017-06-21T07:22:33Z

src/appleseed.studio/mainwindow/mainwindow.cpp

    return false;
 }

+bool MainWindow::can_close_project()


I separated it to use in python module, but don't use it now. Should I revert it?

Yes, if it is unused, lets remove it.

glebmish · 2017-06-21T07:22:55Z

src/appleseed.studio/mainwindow/mainwindow.cpp

+    return m_project_manager.get_project();
+}
+
+bool MainWindow::is_project_dirty()


is it ok to add this function here?

yes, I think so.

maybe change all appearances of m_project_manager.is_project_dirty in MainWindow on this then?

I'm really not enthusiastic about it. I see no value in duplicating what the project manager already offers. I'm in favor of removing this method.

glebmish · 2017-06-21T07:24:37Z

src/appleseed.studio/python/module.cpp

+
+MainWindow* mainwindow()
+{
+    return PythonInterpreter::instance().get_mainwindow();


Does it have any significant overhead to call for interpreter instance every time I need MainWindow?

Not really. Scripting is not really about performance and on top of that Python is super slow.

Should be get_main_window().

dictoon · 2017-06-21T08:45:57Z

src/appleseed.studio/mainwindow/mainwindow.cpp

 }

-bool MainWindow::can_close_project()
+bool MainWindow::attempt_close_project()


The name of this method is misleading: it isn't closing the project, it's just checking if the project can be closed. Maybe ask_can_close_project()? Not ideal but better. Maybe you have a better suggestion? :)

I will revert it back to old can_close_project function as I don't use separated part anyway

dictoon · 2017-06-21T08:46:37Z

src/appleseed.studio/mainwindow/mainwindow.cpp


+bool MainWindow::can_close_project()
+{
+    return !m_project_manager.is_project_loading() && // Project is not being loaded: no problem


Rewrite as a series of if with early exits as it used to be done. This makes the logic much clearer.

same as above

dictoon · 2017-06-21T08:49:31Z

src/appleseed.studio/python/module.cpp

+
+// appleseed.studio headers.
+#include "mainwindow/mainwindow.h"
+#include "pythoninterpreter.h"


Should be python/pythoninterpreter.h.

dictoon · 2017-06-21T09:26:24Z

src/appleseed.studio/python/module.cpp

+using namespace renderer;
+using namespace foundation;
+
+MainWindow* mainwindow()


Should be main_window().

dictoon · 2017-06-21T09:26:35Z

src/appleseed.studio/python/module.cpp

+
+MainWindow* mainwindow()
+{
+    return PythonInterpreter::instance().get_mainwindow();


Should be get_main_window().

dictoon · 2017-06-21T09:38:41Z

src/appleseed.studio/python/pythoninterpreter.cpp

 }

-void PythonInterpreter::execute_command(const char* command)
+void PythonInterpreter::set_mainwindow(MainWindow* window)


Should be set_main_window().

dictoon · 2017-06-21T09:39:11Z

src/appleseed.studio/python/pythoninterpreter.h

    PythonInterpreter();
    ~PythonInterpreter();
+
+    MainWindow* mainWindow;


Should be m_main_window.

dictoon · 2017-06-21T09:39:49Z

Starts to look pretty awesome!

dictoon · 2017-06-22T06:45:36Z

src/appleseed.studio/mainwindow/mainwindow.cpp

-      case QMessageBox::Save:
-        slot_save_project();
-        return true;
+        case QMessageBox::Save:


Revert indentation for the switch block.

dictoon · 2017-06-22T06:46:07Z

src/appleseed.studio/mainwindow/mainwindow.cpp

+    m_project_manager.load_project(filepath.toAscii().constData());
+}
+
 void MainWindow::open_and_render_project(const QString& filepath, const QString& configuration)


I thought you wanted to remove MainWindow::open_and_render_project()?

dictoon · 2017-06-22T06:47:44Z

src/appleseed.studio/mainwindow/mainwindow.cpp

-        update_recent_files_menu(filepath);
-        update_workspace();
-    }
+    save_project(filepath);


There is a regression here: if the user cancelled the Save As dialog, filepath is empty. It appears like this case is no longer handled correctly.

I think there isn't
This check is in save_prject(): if (!filepath.isEmpty())

But it isn't in update_recent_files_menu() if I'm not mistaken.

dictoon · 2017-06-22T06:48:05Z

src/appleseed.studio/mainwindow/mainwindow.h

    // Destructor.
    ~MainWindow();

+    ProjectManager* get_project_manager();


Make the method const.

dictoon · 2017-06-22T06:48:37Z

src/appleseed.studio/python/module.cpp

+    return PythonInterpreter::instance().get_main_window();
+}
+
+ProjectManager* project_manager() {


Move brace to its own line.

dictoon · 2017-06-22T06:48:53Z

src/appleseed.studio/python/module.cpp

+    return project_manager()->get_project();
+}
+
+void save_project(const char* project_path = 0)


Prefer nullptr now that we're using C++11.

dictoon · 2017-06-22T06:48:58Z

src/appleseed.studio/python/module.cpp

+
+void save_project(const char* project_path = 0)
+{
+    if (project_path == 0)


dictoon · 2017-06-22T06:50:10Z

src/appleseed.studio/python/pythoninterpreter.cpp

 namespace bf = boost::filesystem;

+// Function prototypes.
+extern "C" void init_appleseedstudio();


Where is this function defined?

it is generated by BOOST_PYTHON_MODULE(_appleseedstudio) in module.cpp

Got it. Maybe worth adding a comment to explain this.

dictoon · 2017-06-22T06:51:46Z

Almost there! I think you introduced a regression that needs to be fixed, apart from that I've made a handful of minor cosmetics/formatting remarks. Once this is fixed we can merge!

dictoon

Perfect!

…unction in python studio module

* Fix camera movement bug * Fix line endings * Fix issues -Use normalized camera direction -Simplify expressions -Cosmetics

CIE 1960 UCS was removed from the colorTransform node together with the uv related functions. The main purpose here was to get uv chromaticity coordinates from correlated color temperature, but we ended up using Klang (2002) CCT to xy method, which dispenses uv and UCS. HSL, HSV functions were cleaned up and deal correctly with corner cases. RGB->XYZ->RGB roundtrip, when no space change occurred, using only XYZ as an intermediary space for transforms, did incur precision loss which in some cases was enough to set a component as negative (imaginary color). In the case of HSV, HSL transformations, this resulted in a visible artifacts. In short, XYZ/RGB have lower bound restricted to 0.

est77 reviewed Jun 17, 2017

View reviewed changes

dictoon reviewed Jun 18, 2017

View reviewed changes

glebmish commented Jun 21, 2017

View reviewed changes

dictoon reviewed Jun 21, 2017

View reviewed changes

dictoon reviewed Jun 22, 2017

View reviewed changes

dictoon approved these changes Jun 22, 2017

View reviewed changes

glebmish and others added 17 commits June 22, 2017 11:24

Implement studio python module with ability to open project in studio

861c9c4

Implement open new project in studio for python

a972697

Clean up

a54985a

Add ability to fail if old project is not unsaved for new_project() f…

ba2935d

…unction in python studio module

Implement save_project function for python module

610ce19

Expose current_project function to python module

ba98630

Change new_project function to return Project object

d2d32fb

Separate call for MainWindow in function to shorten code

f2db2e0

Change open_project function to return Project object

a040b7d

Implement is_project_dirty function for python module

5ef0bae

Clean up

971b625

Use ProjectManager directly instead or writing wrappers in MainWindow

36179b7

Fix camera movement bug (appleseedhq#1468)

a29f617

* Fix camera movement bug * Fix line endings * Fix issues -Use normalized camera direction -Simplify expressions -Cosmetics

Emit warning message if controller target is off viewing direction

6d9768b

Only update camera's controller target if it has one

9f6f5d3

Add File -> Close Project to appleseed.studio

ee68194

glebmish added 2 commits June 22, 2017 11:24

Implement close_project function for python module

ef5e0cb

Review changes

53f8638

glebmish force-pushed the studio-python-module branch from 6388b68 to 53f8638 Compare June 22, 2017 08:25

Merge branch 'master' into studio-python-module

87434db

glebmish merged commit 1cdad94 into appleseedhq:master Jun 22, 2017

glebmish deleted the studio-python-module branch June 22, 2017 08:53

		@@ -85,7 +98,9 @@ PythonInterpreter::PythonInterpreter()
		// Imports appleseed module and creates asd alias for it


		#include "foundation/platform/python.h"

		#include "pythoninterpreter.h"

Conversation

glebmish commented Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glebmish Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glebmish commented Jun 17, 2017 •

edited

Loading

glebmish Jun 17, 2017 •

edited

Loading