TODOs and hacks remaining in NativeEngine implementation

[syntheticmagus]

• Ensure that ViewClearState::Update is actually setting all the appropriate flags, or open issues/documentation to remedy that later:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.h

  Lines 86 to 92 in 1b40d7b

  	void Update() const
  	{
  	bgfx::setViewClear(m_viewId, m_flags, Color(), m_depth, m_stencil);
  	// discard any previous set state
  	bgfx::discard();
  	bgfx::touch(m_viewId);
  	}

• Consider optimizing FrameBufferManager::Bind by only calling SetUpView when absolutely necessary (which may be as infrequently as on context loss):

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.h

  Lines 175 to 185 in 1b40d7b

  	void Bind(FrameBufferData* data)
  	{
  	m_boundFrameBuffer = data;
  	// TODO: Consider doing this only on bgfx::reset(); the effects of this call don't survive reset, but as
  	// long as there's no reset this doesn't technically need to be called every time the frame buffer is bound.
  	m_boundFrameBuffer->SetUpView(GetNewViewId());
  	// bgfx::setTexture()? Why?
  	// TODO: View order?
  	}

• bgfx currently reorders FrameBuffer submissions according to its own default order. Enforce the provided order so that behavior exactly matches the Web behavior:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.h

  Line 184 in 1b40d7b

  // TODO: View order?

  @CedricGuillemet
• Investigate

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.cpp

  Line 1279 in 8873731

  throw std::exception{/* Does this case even make any sense? */};

• Do we need these enums anymore? If not, remove them: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngineImpl.h#L295-L297 (dead link, enums already removed)
• Remove NativeEngine::Impl::GetEngine once the real mechanism for creating/destroying engines is implemented:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.h

  Line 326 in 1b40d7b

  Napi::Value GetEngine(const Napi::CallbackInfo& info); // TODO: Hack, temporary method. Remove as part of the change to get rid of NapiBridge.

  (@syntheticmagus)
• Consider options for removing extricated bgfx internals from NativeEngine.cpp:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.cpp

  Lines 21 to 23 in 1b40d7b

  	#define BGFX_UNIFORM_FRAGMENTBIT UINT8_C(0x10) // Copy-pasta from bgfx_p.h
  	#define BGFX_UNIFORM_SAMPLERBIT UINT8_C(0x20) // Copy-pasta from bgfx_p.h
  	#define BGFX_RESET_FLAGS (BGFX_RESET_VSYNC | BGFX_RESET_MSAA_X4 | BGFX_RESET_MAXANISOTROPY)

• Discover implications of “num” value on non-D3D11 pipelines and implement/document accordingly: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L115
• Revisit empty bgfx::VertexDecl hack in NativeEngine::Impl::CreateVertexBuffer:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.cpp

  Lines 539 to 543 in 1b40d7b

  	// HACK: Create an empty valid vertex decl which will never be used. Consider fixing in bgfx.
  	bgfx::VertexLayout vertexLayout;
  	vertexLayout.begin();
  	vertexLayout.m_stride = 1;
  	vertexLayout.end();

• Experiment with ways to remove the infamous dFdy hack: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L375-L377 (@bghgary)
• Set the Z offset in SetState:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.cpp

  Lines 786 to 787 in 1b40d7b

  	// TODO: zOffset
  	const auto zOffset = info[1].As<Napi::Number>().FloatValue();

  [captured in Incorrect rendering when renderOverlay is set to true for meshes #1053]
• Stubs: SetZOffset, GetZOffset, SetDepthTest, GetDepthWrite, SetDepthWrite, SetColorWrite. Implement or delete: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L573-L611 @CedricGuillemet
• SetIntArray stubs. Implement or delete: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L634-L660 @CedricGuillemet
• SetFloatArray stubs. Implement or delete: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L679-L698 @CedricGuillemet
• SetMatrix stubs. Implement or delete: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L711-L723 @CedricGuillemet
• SetBool stub. Implement or delete: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L781-L786 @CedricGuillemet
• Mipmaps: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L805 @CedricGuillemet
• Texture sampling, wrap, and anisotropic stubs. Implement or delete: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L902-L926 @CedricGuillemet
• Handle viewport in DrawIndexed: https://github.com/BabylonJS/BabylonNative/blob/master/Library/Source/NativeEngine.cpp#L1009 @CedricGuillemet
• Draw stub. Implement or delete:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.cpp

  Lines 1344 to 1353 in 1b40d7b

  	void NativeEngine::Draw(const Napi::CallbackInfo& info)
  	{
  	const auto fillMode = info[0].As<Napi::Number>().Int32Value();
  	const auto elementStart = info[1].As<Napi::Number>().Int32Value();
  	const auto elementCount = info[2].As<Napi::Number>().Int32Value();
  	// STUB: Stub.
  	// bgfx::submit(), right? Which means we have to preserve here the state of
  	// which program is being worked on.
  	}

  @CedricGuillemet
• Confirm that GetRenderWidth and GetRenderHeight are returning the correct values:

  BabylonNative/Plugins/NativeEngine/Source/NativeEngine.cpp

  Lines 1375 to 1383 in 1b40d7b

  	Napi::Value NativeEngine::GetRenderWidth(const Napi::CallbackInfo& info)
  	{
  	return Napi::Value::From(info.Env(), bgfx::getStats()->width);
  	}
  	Napi::Value NativeEngine::GetRenderHeight(const Napi::CallbackInfo& info)
  	{
  	return Napi::Value::From(info.Env(), bgfx::getStats()->height);
  	}

  @CedricGuillemet
• Fix stub for URL.createObjectURL in engine [captured in Fix stubs for URL.createObjectURL and URL.revokeObjectURL in engine #1240]
• Fix stub for URL.revokeObjectURL in engine [captured in Fix stubs for URL.createObjectURL and URL.revokeObjectURL in engine #1240]
• Fix stub for Blob in engine [captured in Fix stub for Blob in engine #1241]

[CedricGuillemet]

I didn't find an equivalent for SetZOffset, GetZOffset in bgfx. Did I miss something?

[CedricGuillemet]

Discover implications of “num” value on non-D3D11 pipelines -> only used with Vulkan when adding a sampler. I've updated the comment.

[CedricGuillemet]

This issue seems outdated to me. Any objection for closing it? @bghgary @syntheticmagus

[bghgary]

I think many of these are fixed now, but there are still some left that are not. I think we can open separate issues for the remaining items. Thoughts?

[bghgary]

I checked a bunch of the checkboxes now that many of them are addressed. Only the zOffset one, which causes #1053 and stub ones are left.

[bghgary]

@docEdub Do you mind splitting the remaining checkboxes into their own issues?

[docEdub]

The checkbox for "Set the Z offset in SetState:" is captured in issue #1053.

The checkboxes for URL.createObjectURL and URL.revokeObjectURL are now captured in issue #1240.

The checkbox for "Fix stub for Blob in engine" is now captured in issue #1241.

[bghgary]

Added issues to OP, closing in favor of separate issues.