Usability changes and fixes for errors and warnings

[GSORF]

Hi @maximeraafat,

First of all thank you very much for providing this addon! I speeds up the process of creating datasets tremendously and I have successfully tested it with InstantNGP.

During my tests, I have noticed some issues regarding usablity and therefore propose the following changes in this PR:

• [x] Changed "Train" and "Test" buttons to an EnumProperty (Radio buttons) in order to allow the selection of either training or testing set generation. I did this due to the missing functionality to export testing images. But now after implementing this, I saw that this seems to be intended behaviour, right?
• [x] Fixed error when when 'empty_fn' was assigned to 'properties_ui_upd(self, context)' in 'helper.py' - it expected two arguments (self and context)
• [x] Fixed warning by renam in ui panel 'idname' to include 'PT' and only consist of alphanumeric values (i.e. removed '.').
• [x] Furthermore renamed 'PLAY {MOTION-OPERATOR}' to 'EXPORT {MOTION-OPERATOR}' for the different panel user interfaces for "COS", "SOF" and "TTC".

Regarding the first point with the introduction of a new enumProperty, I am not sure if my fix is to your liking. I suspect that I misunderstood the usage of the addon here, since with my fixes it is NOT possible to include both Training and Testing datasets but they are mutually exclusive now. A user would need to unzip the "dataset.zip" file now and merge both files. Furthermore, I noticed that you are not rendering the test images - is there a reason for this?

I would appreciate if you could take a look at my suggestions and cherrypick the ones you like or simply decline this pull request if you don't agree with them.

Thank you and kind regards, Adam

[maximeraafat]

Hi Adam, thank you for taking the time to suggest all these great code changes!

• Regarding the first suggestion, this is indeed intended behaviour. While the add-on is very useful for research purposes, it was primarily intended as a tool to render test images instead of using them for some kind of evaluation. The goal is for the user not to actually have to render test images, but instead to directly render any viewpoint from a test camera path. This is why I did not include a functionality to render test images. They can however very simply be rendered in one additional step, by selecting the desired test camera and pressing the Render Animation command within Blender.

• Regarding the last suggestion, I have also considered renaming the operators simply to EXPORT. But in order to avoid ambiguity, I left it as is (PLAY {METHOD_NAME}). Purely from a linguistic perspective, EXPORT {METHOD_NAME} would be less accurate that PLAY {METHOD_NAME}, since I am exporting the rendered result and camera information from the operator, and not the operator itself. To keep the add-on as friendly as possible to new beginners, I would prefer keeping the current nomenclature.

• Finally, as for the remaining two points, I will investigate your code fixes in more details very soon, and include them (if indeed helpful) into the next version of the add-on that I will release in the next coming days (given that this is okay with you). If I end up not adding them to the next release, I will of course let you know of my reasons in this conversation.

Thank you again for your contributions! I hope this reply answers your questions and helps you understand some of the design choices that I made. Feel free to ask more questions here or to open a new issue! :)

Kind regards, Maxime

[GSORF]

Hi @maximeraafat,

Thank you for your fast answer!

I understand your feedback and your concept of generating a test dataset. However, just wanted to let you know that the behaviour was a bit surprising from my view (i.e. looked like a bug). But yeah, you are in fact mentioning this in the README, so if someone just reads after implementing a new thing, you cannot do anything about it ;)

About the nomenclature: Oh, I see what you mean. But running the operators is not playing them either, right? Because when you are i.e. generating a testing data set, you are not playing anything but rather creating the transforms.json file. Maybe CREATE {METHOD_NAME} DATASET would be a compromise you could agree on? ;)

Well, these are just suggestions. I completely get your points and you should keep your initial design choices of course.

If you find anything helpful in this PR just feel free to add it your releases (as that's why I opened the PR in the first place ;) ). You can also just close it and incoorporate specific changes yourself. Thanks for the valuable discussion about these points.

Keep up the great work and kind regards, Adam

[maximeraafat]

Hi @GSORF,

I would argue that the nomenclature does not matter too much, as long as it does not affect the ease of usability. I like it in this simple and short way, and will therefore keep it as is for now. I will however update the README to further clarify the purposes of the PLAY {METHOD_NAME} operators.

Regarding your remaining suggestions, could you please enlighten me on the warnings you have been experiencing?

1. You renamed the bl_idname properties in the panel classes, for example from panel.blender_nerf_ui to SCENE_PT_blender_nerf_ui in the BlenderNeRF_UI class. I have never encountered any warning from this, but I am also no expert of the Blender python API. After some quick googling, I have however noticed that there is a naming convention, and I should instead call it VIEW3D_PT_blender_nerf_ui according to this page. Regardless, I would appreciate if you could share with me the warnings you have been seeing!
2. In the helper.py script, I assign the empty_fn function to both can_scene_upd and can_properties_upd when upd_off is called. can_scene_upd takes indeed two arguments (self and context), but can_properties_upd only takes a single argument (scene). These functions however only take arguments when calling the upd_on function, and I believe it should therefore not matter how many arguments are passed to empty_fn. If you have experienced issues or warnings from this, I would be thankful if you could share these with me.

Thank you again for the suggestions, the feedback is of great value to me and I appreciate any contribution to this project :)

Cheers, Maxime

[GSORF]

Hi Maxime,

sure! The warning I got regarding the _PT_ prefix was the following:

register_class(...):
Warning: 'panel.blender_nerf_ui' does not contain '_PT_' with prefix and suffix
register_class(...):
Warning: 'panel.sof_ui' does not contain '_PT_' with prefix and suffix
register_class(...):
Warning: 'panel.ttc_ui' does not contain '_PT_' with prefix and suffix
register_class(...):
Warning: 'panel.cos_ui' does not contain '_PT_' with prefix and suffix

This is with Blender version 3.5.0 (Alpha) on windows now, using your BlenderNeRF release 4.0.0 from Feb 12. I was, however, creating my Pull Request using Blender 3.5.1 on Ubuntu Linux - same warnings. Warning appears once I enable the addon.

And you are right! The bl_idname should be named VIEW3D_PT_blender_nerf_ui instead of SCENE_PT_blender_nerf_ui. I was using the shipped python template blindly :) Sorry about that.

About the issue with the parameter list - the following error appears once I click on "Sphere" after opening the "COS" panel:

Traceback (most recent call last):
  File "C:\Users\{USER}\AppData\Roaming\Blender Foundation\Blender\3.5\scripts\addons\BlenderNeRF-4\helper.py", line 112, in properties_ui_upd
    can_scene_upd(self, context)
TypeError: empty_fn() takes 1 positional argument but 2 were given
File "C:\Users\{USER}\AppData\Roaming\Blender Foundation\Blender\3.5\scripts\addons\BlenderNeRF-4\helper.py", line 111, in properties_ui_upd

[... repeats four times in total, per button click]

Hope this helps.

Best regards, Adam

[maximeraafat]

Hi Adam,

Hmm very strange, I don't get any of these warnings or errors on macOS, even when uninstalling and reinstalling the add-on from the exact same version (v4.0.0). Perhaps this is due to the different underlying compilers? When launching the python console in Blender, I see the following message:

Perhaps different versions of python interpreters might also be the cause for this difference? I am unsure. Anyways, thanks a lot for pointing these things out!

I will change the id name according to the common convention. As for the error related to the empty_fn function, I will simply add the second (context) argument to the function. If any issues remain, feel free to inform me!

I will push these changes very soon as part of the next release :)

Thanks again, Maxime

[GSORF]

Hi Maxime,

just in case that this is the reason: The warning / error will not be printed in the python console within Blender - you need to open Blender via the command line on Linux and Mac OS. On Windows you open the Console Window via "Window - Toggle System Console". Then you should see the warnings there, I wouldn't expect MacOS to behave differently here. But you probably know that.

A quick way to test that is to make a small reproducible example using the templates shipped with Blender.

For example, this will not print a warning to the console on Windows:

import bpy

class LayoutDemoPanel(bpy.types.Panel):
    """Creates a Panel in the scene context of the properties editor"""
    bl_label = "Layout Demo"
    bl_idname = "SCENE_PT_layout"
    bl_space_type = 'PROPERTIES'
    bl_region_type = 'WINDOW'
    bl_context = "scene"

    def draw(self, context):
        layout = self.layout

        scene = context.scene

        # Create a simple row.
        layout.label(text=" Simple Row:")

        row = layout.row()
        row.prop(scene, "frame_start")
        row.prop(scene, "frame_end")

        # Create an row where the buttons are aligned to each other.
        layout.label(text=" Aligned Row:")

        row = layout.row(align=True)
        row.prop(scene, "frame_start")
        row.prop(scene, "frame_end")

        # Create two columns, by using a split layout.
        split = layout.split()

        # First column
        col = split.column()
        col.label(text="Column One:")
        col.prop(scene, "frame_end")
        col.prop(scene, "frame_start")

        # Second column, aligned
        col = split.column(align=True)
        col.label(text="Column Two:")
        col.prop(scene, "frame_start")
        col.prop(scene, "frame_end")

        # Big render button
        layout.label(text="Big Button:")
        row = layout.row()
        row.scale_y = 3.0
        row.operator("render.render")

        # Different sizes in a row
        layout.label(text="Different button sizes:")
        row = layout.row(align=True)
        row.operator("render.render")

        sub = row.row()
        sub.scale_x = 2.0
        sub.operator("render.render")

        row.operator("render.render")

def register():
    bpy.utils.register_class(LayoutDemoPanel)

def unregister():
    bpy.utils.unregister_class(LayoutDemoPanel)

if __name__ == "__main__":
    register()

But this will print the warning

register_class(...):
Warning: 'panel.layout' does not contain '_PT_' with prefix and suffix

on Windows due to bl_idname = "panel.layout":

import bpy

class LayoutDemoPanel(bpy.types.Panel):
    """Creates a Panel in the scene context of the properties editor"""
    bl_label = "Layout Demo"
    bl_idname = "panel.layout"
    bl_space_type = 'PROPERTIES'
    bl_region_type = 'WINDOW'
    bl_context = "scene"

    def draw(self, context):
        layout = self.layout

        scene = context.scene

        # Create a simple row.
        layout.label(text=" Simple Row:")

        row = layout.row()
        row.prop(scene, "frame_start")
        row.prop(scene, "frame_end")

        # Create an row where the buttons are aligned to each other.
        layout.label(text=" Aligned Row:")

        row = layout.row(align=True)
        row.prop(scene, "frame_start")
        row.prop(scene, "frame_end")

        # Create two columns, by using a split layout.
        split = layout.split()

        # First column
        col = split.column()
        col.label(text="Column One:")
        col.prop(scene, "frame_end")
        col.prop(scene, "frame_start")

        # Second column, aligned
        col = split.column(align=True)
        col.label(text="Column Two:")
        col.prop(scene, "frame_start")
        col.prop(scene, "frame_end")

        # Big render button
        layout.label(text="Big Button:")
        row = layout.row()
        row.scale_y = 3.0
        row.operator("render.render")

        # Different sizes in a row
        layout.label(text="Different button sizes:")
        row = layout.row(align=True)
        row.operator("render.render")

        sub = row.row()
        sub.scale_x = 2.0
        sub.operator("render.render")

        row.operator("render.render")

def register():
    bpy.utils.register_class(LayoutDemoPanel)

def unregister():
    bpy.utils.unregister_class(LayoutDemoPanel)

if __name__ == "__main__":
    register()

I have just tested above scripts on my M1 Macbook Pro (but in Blender 3.3.0) and it throws the same warning as on Linux and Windows. If you have time, you might want to check.

Thanks for adding my fixes to the main branch.

Best regards, Adam

[maximeraafat]

Hi Adam,

Thanks for clarifying this, in the command line I now see the warning indeed!

Cheers, Maxime