Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data exchange cameras for 3d Studio Max #4376

Merged

Conversation

moonyuet
Copy link
Member

@moonyuet moonyuet commented Jan 26, 2023

Brief description

Add Camera Family into the 3d Studio Max

Description

Adding Camera Extractors(extract abc camera and extract fbx camera) and validators(for camera contents) into 3dMax
Also add the extractor for exporting 3d max raw scene (which is also related to 3dMax Scene Family) for camera family

Additional info

When assigning the instance to the selected objects, it will pop up the panel to ask you if you are okay for parenting the selected items into the container, you need to click yes.

image

Testing notes:

  1. Launch 3dmax with Openpype
  2. go to Openpype > Publish
  3. Create Camera Instance with the camera item
  4. Publish

@moonyuet moonyuet added type: feature Larger, user affecting changes and completely new things host: 3dsmax Autodesk 3dsmax labels Jan 26, 2023
@moonyuet moonyuet self-assigned this Jan 26, 2023
@jakubjezek001
Copy link
Member

Task linked: OP-4244 Data Exchange: cameras

Copy link
Member

@antirotor antirotor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add simple settings for the extractors so by enabling/disabling them you would set what formats you want to use.

Also beware that it is pulling changes to settings not related to this PR - probably something leaked from the path validator.

Comment on lines 38 to 47
for sel in selection_list:
# to avoid Attribute Error from pymxs wrapper
sel_tmp = str(sel)
for cam in self.camera_type:
if sel_tmp.startswith(cam):
validation_msg.append("Camera Found")
else:
validation_msg.append("Camera Not Found")
if "Camera Found" not in validation_msg:
invalid.append(sel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would simplify this:

Suggested change
for sel in selection_list:
# to avoid Attribute Error from pymxs wrapper
sel_tmp = str(sel)
for cam in self.camera_type:
if sel_tmp.startswith(cam):
validation_msg.append("Camera Found")
else:
validation_msg.append("Camera Not Found")
if "Camera Found" not in validation_msg:
invalid.append(sel)
for sel in selection_list:
# to avoid Attribute Error from pymxs wrapper
sel_tmp = str(sel)
found = False
for cam in self.camera_type:
if sel_tmp.startswith(cam):
found = True
break
if not found:
self.log.error("Camera not found")
invalid.append(sel)

Copy link
Collaborator

@BigRoy BigRoy Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think check code-wise is odd to read.

We seem to be comparing the object type to see if it's a camera.

I'd search on how to explicitly get the object type instead of assuming it's the start of the string for str(object) of a 3ds max object so that we could turn the whole validation .py file into something along these lines:

# -*- coding: utf-8 -*-
import pyblish.api
from openpype.pipeline import PublishValidationError
from pymxs import runtime as rt


def is_valid_camera_node(node):
    obj_type = node.type()  # or whatever the valid method is for that
    return obj_type in {"$Free_Camera", "$Target_Camera", 
                        "$Physical_Camera", "$Target"}
    

class ValidateCameraContent(pyblish.api.InstancePlugin):
    """Validates Camera instance contents.

    A Camera instance may only hold a SINGLE camera's transform
    """

    order = pyblish.api.ValidatorOrder
    families = ["camera"]
    hosts = ["max"]
    label = "Camera Contents"

    def process(self, instance):
        invalid = self.get_invalid(instance)
        if invalid:
            raise PublishValidationError("Camera instance must only include"
                                         "camera (and camera target)")

    def get_invalid(self, instance):
        """Get invalid nodes if the instance is not camera"""
        
        container = rt.getNodeByName(instance.data["instance_node"])

        invalid = list()
        for child in list(container.Children):
            if not is_valid_camera_node(child):
                invalid.append(child)
                
        return invalid

Potentially even:

        invalid = [obj for obj in container.Children if not is_valid_camera_node(obj)]

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had another quick peek over the 3ds Max pymxs docs. I think it should be something along these lines:

One of these:

rt.superClassOf(node) 
rt.classOf(node)

And then check it against the actual rt types:

def is_valid_camera_node(node):
    obj_type = rt.classOf(node)
    return obj_type in {rt.Free_Camera, rt.Target_Camera,
                        rt.Physical_Camera, rt.Target}

Having never used 3ds Max I'm not sure that's anywhere remotely valid - but it seemed like that should be roughly it. :) But do consider it pseudocode!

Comment on lines 51 to 55
def list_children(self, node):
children = []
for c in node.Children:
children.append(c)
return children
Copy link
Member

@antirotor antirotor Jan 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldn't this be replaced with list(node.Children)?

instance.data["representations"] = []

# add extra blacklash for saveNodes in MaxScript
re_max_path = stagingdir + "\\\\" + filename
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is done for double backslashes, maybe something like:

re_max_path = os.path.normpath(max_path).replace("\\", "\\\\")

to be sure that they are everywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most apps also deal with it fine if you force forward slashes instead - it usually makes the output easier to read so if it works I'd recommend:

os.path.normpath(max_path).replace("\\", "/")

@ynput ynput deleted a comment from moonyuet Jan 26, 2023
@BigRoy
Copy link
Collaborator

BigRoy commented Jan 26, 2023

afbeelding

Ha - this sounds really mean. But it was basically @moonyuet and me replying to each other about an image/comment he originally already deleted. So I ended up deleting the conversation since it was basically impossible to follow along in the end.

@moonyuet
Copy link
Member Author

moonyuet commented Jan 26, 2023

The updated PR will clean up the cliche and also allows enable/disable functions for the extractors in the publish panel
image

"""

order = pyblish.api.ExtractorOrder - 0.1
label = "Extract Almebic Camera"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
label = "Extract Almebic Camera"
label = "Extract Alembic Camera"

)


class ExtractCameraFbx(publish.Extractor,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it weird maybe that this is ExtractCameraFbx but the other is ExtractAlembicCamera? :)

Should they either both be:

ExtractCameraAlembic
ExtractCameraFbx

or:

ExtractFbxCamera
ExtractAlembicCamera

@LiborBatek
Copy link
Member

Did run a test of camera publish and got this error tied with Colorspace. Should I test differently? I guess its a new feature incorporated in OP 3.15

image

otherwise the Camera options shows up:
image

@LiborBatek
Copy link
Member

LiborBatek commented Jan 30, 2023

Regarding the failure during publishing maybe got already fixed with this #4390 ? @moonyuet maybe its needed in your branch to not fail on colorspace data ??

@moonyuet
Copy link
Member Author

Regarding the failure during publishing maybe got already fixed with this #4390 ? @moonyuet maybe its needed in your branch to not fail on colorspace data ??

I have merged the latest develop to this branch. Thanks!

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 30, 2023

Regarding the failure during publishing maybe got already fixed with this #4390 ? @moonyuet maybe its needed in your branch to not fail on colorspace data ??

It won't be fixed with #4390 since it still assume the host implementation has its own OCIO settings, see code here.

It should either allow the host implementation to NOT have that implemented (I'd prefer that actually!).
Or it would require us to implement OCIO for max to be supported correctly.

@jakubjezek001

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works normally and publish all representations as set during Publish.
image

image

@antirotor antirotor merged commit ce449f9 into ynput:develop Jan 31, 2023
@antirotor
Copy link
Member

OCIO support has to be addressed, but in another PR

@github-actions github-actions bot added this to the next-patch milestone Jan 31, 2023
@moonyuet moonyuet deleted the feature/OP-4244-Data-Exchange-Cameras branch January 31, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: 3dsmax Autodesk 3dsmax type: feature Larger, user affecting changes and completely new things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants