Update animated parameters by getting current time from renderer by usakhelo · Pull Request #234 · appleseedhq/appleseed-max

usakhelo · 2018-10-14T05:01:24Z

Hi,
This PR fixes bug #212. OSL materials and textures were not updated during animation rendering. Now they are working. What I did is instead of getting current with the call GetCOREInterface()->GetTime() I added a function to renderer to get currently rendered frame from the it.

Thanks for review!
Sergo.

dictoon · 2018-10-14T08:05:24Z

 {
    float sun_theta_deg = m_sun_theta;
    float sun_phi_deg = m_sun_phi;
+    const TimeValue time = get_current_time();


Move inside the if below?

dictoon · 2018-10-14T08:07:16Z


 void OSLMaterial::Update(TimeValue t, Interval& valid)
 {
+


Remove redundant blank line.

dictoon · 2018-10-14T08:08:28Z

    auto shader_group_name = make_unique_name(assembly.shader_groups(), std::string(name) + "_shader_group");
    auto shader_group = asr::ShaderGroupFactory::create(shader_group_name.c_str());
+
+    const TimeValue time = get_current_time();


Move inside if below?

dictoon · 2018-10-14T08:08:39Z

+            else
+                m_pblock->GetValue(tex_param.first, t, tex_map, m_params_validity);
+
+            if (tex_map)


Make comparison explicit.

dictoon · 2018-10-14T08:09:06Z

+        {
+            Texmap* tex_map = nullptr;
+            m_pblock->GetValue(tex_param.first, t, tex_map, m_params_validity);
+            if (tex_map)


Make comparison explicit.

dictoon · 2018-10-14T08:10:51Z

    Texmap*             texmap)
 {
-    const auto t = GetCOREInterface()->GetTime();
+    TimeValue time = get_current_time();


Make const.

dictoon · 2018-10-14T08:11:07Z

    const float         const_value)
 {
-    const auto t = GetCOREInterface()->GetTime();
+    const auto time = get_current_time();


Use TimeValue out of consistency with the rest of the code.

dictoon · 2018-10-14T08:11:10Z

    const Color         const_value)
 {
-    const auto t = GetCOREInterface()->GetTime();
+    TimeValue time = get_current_time();


Make const.

dictoon · 2018-10-14T08:11:36Z

+        Texmap* source_map;
+        m_pblock->GetValue(ParamIdSourceMap, t, source_map, m_params_validity);

+        if (source_map)


Make comparison explicit.

dictoon · 2018-10-14T08:18:26Z


 // appleseed-max headers.
 #include "appleseedoslplugin/osltexture.h"
+#include "appleseedrenderer/appleseedrenderer.h"


I find it a bit concerning that utilities depends on appleseedrenderer: lower level components should never depend on higher level ones. Maybe get_current_time() should be declared in appleseedrenderer.{h,cpp}.

For the same reason I'm slightly concerned about the dependencies toward appleseedoslplugin/osltexture.h and osloutputselectormap/osloutputselector.h.

Can I make get_current_time in appleseedrenderer a global function, not the class member? So that I don't have to check and cast current renderer to get the time.

Yes, that's what I meant. You could also make it a static method of AppleseedRenderer to highlight the dependency to that class.

I'm trying to move it to the renderer class but in some utilities I still need to call that function so I still need to include renderer header.

Other way is to pass time from the renderer to all that create_material and so on as an argument.

usakhelo · 2018-10-15T00:32:41Z

Hi @dictoon ,
I added time parameter to the utility functions and then I noticed that only couple of other functions need to be changed in order to get rid of static get_time function. So I decided to change all the functions and now time is passed from the project builder to each material creation function.
It may seem too much of parameters to some of the functions, but it also makes it obvious that they all need time information to work correctly.

luisbarrancos · 2018-10-15T02:44:43Z

This might be relevant:
AcademySoftwareFoundation/OpenShadingLanguage#828
OSL will have acess to global (or camera) frame.

dictoon · 2018-10-15T20:48:10Z

So I decided to change all the functions and now time is passed from the project builder to each material creation function.

In my opinion, definitely the right solution as it makes dependencies clear.

dictoon

Looking good! Feel free to merge after you've addressed the few remaining details I reported.

dictoon · 2018-10-15T20:50:53Z

    m_entities.clear();
 }

-


We conventionally keep two blank lines before block comments.

dictoon · 2018-10-15T20:52:50Z

    {
      public:
-        explicit MaxShadeContext(const asr::SourceInputs& source_inputs)
+        explicit MaxShadeContext(const asr::SourceInputs& source_inputs, TimeValue time)


explicit has become redundant here, not that it really hurts...

Make time const.

dictoon · 2018-10-15T20:53:11Z

    {
      public:
-        explicit MaxProceduralTextureSource(Texmap* texmap)
+        explicit MaxProceduralTextureSource(Texmap* texmap, TimeValue time)


Make time const.

dictoon · 2018-10-15T20:53:26Z

      private:
-        Texmap* m_texmap;
+        Texmap*     m_texmap;
+        TimeValue   m_time;


Can it be made const here too? (I think so.)

dictoon · 2018-10-15T20:53:49Z

    {
      public:
-        MaxProceduralTexture(const char* name, Texmap* texmap)
+        MaxProceduralTexture(const char* name, Texmap* texmap, TimeValue time)


Make time const.

Update animated parameters by getting current time from renderer

edbb212

dictoon requested changes Oct 14, 2018

View reviewed changes

Pass time parameter from renderer to material and texture functions

8b13cec

dictoon approved these changes Oct 15, 2018

View reviewed changes

Fix comments

9ef6c08

usakhelo merged commit ddc03b0 into appleseedhq:master Oct 16, 2018

usakhelo deleted the fix_animated_parameters branch October 14, 2019 04:45

Conversation

usakhelo commented Oct 14, 2018

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!

usakhelo commented Oct 15, 2018

Uh oh!

luisbarrancos commented Oct 15, 2018

Uh oh!

dictoon commented Oct 15, 2018

Uh oh!

dictoon left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants