Bug 233265 – [Model] add support for getting and setting the camera

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

[Model] add support for getting and setting the camera

[Antoine Quint]

Created attachment 444544 [details]
Patch

[Antoine Quint]

<rdar://problem/85426290>

[Darin Adler]

Comment on attachment 444544 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444544&action=review

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:304
> +    m_modelPlayer->getCamera([protectedThis = Ref { *this }, promise = WTFMove(promise)] (std::optional<HTMLModelElementCamera> camera) mutable {

I don’t fully understand why protectedThis is helpful. Why is extending the lifetime of the HTMLModelElement valuable, given we do nothing with that element pointer?

> Source/WebCore/Modules/model-element/HTMLModelElement.cpp:319
> +    m_modelPlayer->setCamera(camera, [protectedThis = Ref { *this }, promise = WTFMove(promise)] (bool success) mutable {

Ditto.

> Source/WebCore/Modules/model-element/HTMLModelElementCamera.h:67
> +    return { {
> +        WTFMove(*pitch),
> +        WTFMove(*yaw),
> +        WTFMove(*scale),
> +    } };

Moving a double isn’t valuable.

    return { { *pitch, *yaw, *scale } };

> Source/WebCore/Modules/model-element/HTMLModelElementCamera.idl:33
> +    double yaw;
> +    double pitch;
> +    double scale;

This says "yaw, pitch, scale", but the C++ says "pitch, yaw, scale". Does the order matter? Could we be consistent?

> Source/WebKit/Shared/ModelIdentifier.h:69
> +    return { { WTFMove(*layerIdentifier) } };

No need to move a layer ID:

    return { { *layerIdentifier } };

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:58
> +WKModelView* ModelElementController::modelViewForModelIdentifier(ModelIdentifier modelIdentifier)

WKModelView * with a space before the *?

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:61
> +        return nullptr;

nil is preferred over nullptr for Objective-C object pointers, I think

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:64
> +        return nullptr;

Ditto.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:68
> +        return nullptr;

Ditto.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:77
>      auto *view = node->uiView();
>      if (!view)
> -        return;
> +        return nullptr;
>  
> -    if (![view isKindOfClass:[WKModelView class]])
> -        return;
> +    if ([view isKindOfClass:[WKModelView class]])
> +        return (WKModelView *)view;
> +
> +    return nullptr;

WebKit has a function template for this that collapses these 8 lines into one:

    return dynamic_objc_cast<WKModelView>(node->uiView());

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:84
> +    if (auto *modelView = modelViewForModelIdentifier(modelIdentifier))
> +        return [modelView preview];
> +    return nullptr;

Objective-C has the nil handling that simplifies this. Can write one of these instead:

    return modelViewForModelIdentifier(modelIdentifier).preview;

    return [modelViewForModelIdentifier(modelIdentifier) preview];

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:93
> +    auto *modelView = modelViewForModelIdentifier(modelIdentifier);

I suggest auto here. I don’t think "auto *" is helpful. Or you could use "auto*" or "WKModelView *".

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:152
> +    if (auto preview = m_inlinePreviews.get(modelIdentifier.uuid))
> +        return preview.get();
> +
> +    return nullptr;

No need for an if here:

    return m_inlinePreviews.get(modelIdentifier.uuid).get();

But this makes me realize we have an unsafe-looking thing here. Normally it would not be safe to call get() on a smart pointer and then just return the raw pointer. The ".get()" here looks unsafe.

What makes it safe is the whole "peek" situation where the map itself holds a RetainPtr, but it seems we didn’t implement PeekType for RetainPtr the way we did for RefPtr. If we had, then HashMap::get would return a raw pointer and we would not need "get()".

[Antoine Quint]

Thanks for all the feedback Darin, this taught me a few Obj-C trick. I'll integrate all of this in the patch for landing. There's one question I can answer:

(In reply to Darin Adler from comment #3)
> Comment on attachment 444544 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444544&action=review
> 
> > Source/WebCore/Modules/model-element/HTMLModelElementCamera.idl:33
> > +    double yaw;
> > +    double pitch;
> > +    double scale;
> 
> This says "yaw, pitch, scale", but the C++ says "pitch, yaw, scale". Does
> the order matter? Could we be consistent?

The order doesn't matter because there is no constructor for an IDL dictionary per-se, instead in JS the author will have to write something like { yaw: 1, pitch: 1, scale: 1 }. But consistency doesn't hurt so I moved the members around to match HTMLModelElementCamera.h.

[Antoine Quint]

Created attachment 444646 [details]
Patch for EWS

[Antoine Quint]

Created attachment 444648 [details]
Patch for EWS

[Antoine Quint]

Created attachment 444651 [details]
Patch for EWS

[Antoine Quint]

Created attachment 444653 [details]
Patch for EWS

[Antoine Quint]

Created attachment 444659 [details]
Patch for EWS

[Antoine Quint]

Created attachment 444687 [details]
Patch for landing

[Antoine Quint]

Created attachment 444691 [details]
Patch for landing

[Antoine Quint]

Created attachment 444698 [details]
Patch for landing

[Antoine Quint]

Committed r286019 (244408@main): <https://commits.webkit.org/244408@main>

[Chris Dumez]

This seems to have broken the build for me:
```
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource34-mm.mm:4:
/Volumes/Data/WebKit/OpenSource/Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:256:14: error: instance method '-getCameraTransform:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]
    [preview getCameraTransform:makeBlockPtr([weakThis = WeakPtr { *this }, completionHandler = WTFMove(completionHandler)] (simd_float3 cameraTransform, NSError *error) mutable {
             ^~~~~~~~~~~~~~~~~~
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource34-mm.mm:4:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:51:
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/pal/spi/mac/SystemPreviewSPI.h:29:
/Volumes/Xcode13A6201j_m20A2411_m21C29_i19C35_FastSim_Boost_Encrypted_54GB/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.Internal.sdk/System/Library/PrivateFrameworks/AssetViewer.framework/PrivateHeaders/ASVInlinePreview.h:39:12: note: receiver is instance of class declared here
@interface ASVInlinePreview : NSObject
           ^
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource34-mm.mm:4:
/Volumes/Data/WebKit/OpenSource/Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:283:14: error: instance method '-setCameraTransform:' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]
    [preview setCameraTransform:simd_make_float3(camera.pitch, camera.yaw, camera.scale)];
             ^~~~~~~~~~~~~~~~~~
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource34-mm.mm:4:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:51:
In file included from /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/usr/local/include/pal/spi/mac/SystemPreviewSPI.h:29:
/Volumes/Xcode13A6201j_m20A2411_m21C29_i19C35_FastSim_Boost_Encrypted_54GB/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX12.1.Internal.sdk/System/Library/PrivateFrameworks/AssetViewer.framework/PrivateHeaders/ASVInlinePreview.h:39:12: note: receiver is instance of class declared here
@interface ASVInlinePreview : NSObject
           ^
2 errors generated.
```

[Antoine Quint]

Committed r286023 (244412@main): <https://commits.webkit.org/244412@main>