search

luxonis / depthai-core

DepthAI C++ Library
MIT License
235 stars 127 forks source link

Remove build, update exampled, replace build with buildInternal #1074

Closed lnotspotl closed 3 months ago

lnotspotl commented 3 months ago

Q: Do we want to remove build completely or deprecate it and print out a warning if it is used?

Q: I don't really like the fact that DeviceNode's buildInternal virtual method is public. Is there a better way to go about this?

moratom commented 3 months ago

Q: Do we want to remove build completely or deprecate it and print out a warning if it is used? Let's remove it, we're still early in the alpha release, just write in the internal channels that we've done it.

Q: I don't really like the fact that DeviceNode's buildInternal virtual method is public. Is there a better way to go about this? What happens if made private?

lnotspotl commented 3 months ago

@moratom Designating the buildInternal method as private would not help here. After the shared pointer nodePtr gets created, calling the buildInternal method is not possible due to it being private.

template <typename... Args>
[[nodiscard]] static std::shared_ptr<Derived> create(std::shared_ptr<Device> device, Args&&... args) {
    auto nodePtr = std::shared_ptr<Derived>(new Derived(device, std::forward<Args>(args)...));
    nodePtr->buildInternal();
    return nodePtr;
}

This can be fixed by marking DeviceNodeCRTP<Base, Derived, Prop> as a friend for each of the classes (ImageManip, XLinkOut, SystemLogger, ...). I am not a big fan of this approach, as you would have two places with e.g. DeviceNodeCRTP<DeviceNode, ImageManip, ImageManipConfig>.

class SystemLogger : public DeviceNodeCRTP<DeviceNode, SystemLogger, SystemLoggerProperties> {

...
  protected:
    friend class DeviceNodeCRTP<DeviceNode, SystemLogger, SystemLoggerProperties>;
    void buildInternal();

...

};

I don't see any other way of doing that. Does something come to your mind?

moratom commented 3 months ago

@lnotspotl let's keep it public for now then, not the cleanest but it was public in rvc3_develop already and it does have Internal in the name.

moratom commented 3 months ago

We should check that the python examples don't call the .build function any more and then we can merge.

lnotspotl commented 3 months ago

No build() function calls anywhere throughout the codebase. The only exception is the Pipeline class, but this PR is not about that, and we want to keep it the way it is anyway. This PR is ready to be merged.

Action for this PR: https://github.com/luxonis/depthai-core/actions/runs/10097705821