Skip to content

More validation tests - #415

Merged
CedricGuillemet merged 40 commits into
BabylonJS:masterfrom
CedricGuillemet:moreValidationTests
Sep 29, 2020
Merged

More validation tests#415
CedricGuillemet merged 40 commits into
BabylonJS:masterfrom
CedricGuillemet:moreValidationTests

Conversation

@CedricGuillemet

@CedricGuillemet CedricGuillemet commented Sep 18, 2020

Copy link
Copy Markdown
Collaborator

Because I like to make your life more difficult by adding validation tests ;)

  • 26 more tests mostly gltf related
  • the validation test errorcode return was wrong. because of a wrong WM_
  • error tolerance boosted to 8% or 5% because of missing text in some scene.
  • success/failure task condition to push the appropriate artifact (results/errors)
  • Y flipped for RTT fixes Support render target textures #87
  • Post process!

bjs max with commit BabylonJS/Babylon.js@07ee198

Comment thread Plugins/NativeEngine/Source/NativeEngine.cpp Outdated
var justOnce;
var threshold = 25;
var errorRatio = 2.5;
var errorRatio = 8.0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this higher than what Babylon.js uses now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes! 2.5 bjs 8 here. Mostly because text is rendered in some PG.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm assuming you mean text is not being rendered because we don't have text rendering yet?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes! That's correct.

@bghgary bghgary Sep 21, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We discussed this offline. The plan is to update this value only for the tests that have text and put a comment somewhere that explains why (or, at a minimum, make sure the commit message says why).

@CedricGuillemet

Copy link
Copy Markdown
Collaborator Author

Adding a bunch more test scenes this week. Back to draft.

@CedricGuillemet
CedricGuillemet marked this pull request as draft September 22, 2020 15:14
…nto missingNativeFunctions

# Conflicts:
#	Apps/BabylonScripts/babylon.max.js
#	Plugins/NativeEngine/Source/NativeEngine.cpp
#	Plugins/NativeEngine/Source/NativeEngine.h
@CedricGuillemet
CedricGuillemet marked this pull request as ready for review September 25, 2020 14:33

@bghgary bghgary left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Many of these changes are not ideal and should have issues associated with them. The API macros thing should be removed completely, but maybe after my changes.

break;
}
case WM_DESTROY:
case WM_CLOSE:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure about this one. I'm assuming you are seeing some shutdown issues. Usually the sequence is WM_CLOSE calls DestroyWindow which then causes WM_DESTROY which calls PostQuitMessage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Replacing with direct errorCode for postquitmessage instead of using message param.

Comment thread Apps/ValidationTests/Win32/App.cpp Outdated
case WM_DESTROY:
case WM_CLOSE:
{
short exitCode = LOWORD(wParam);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by this. Documentation says that the lParam and wParam of both WM_DESTROY or WM_CLOSE are not used. What does this do exactly?

Comment on lines +55 to +56
target_compile_definitions(NativeEngine
PRIVATE API${GRAPHICS_API}) # OpenGL is defined in bgfx.h. Using APIXXX instead

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should probably be in all CAPS to match typical macros which may avoid the conflict. Use https://cmake.org/cmake/help/latest/manual/cmake-generator-expressions.7.html#string-transformations to convert to uppercase. But ideally, this shouldn't be done at all. The specific code should be in the specific files instead of using macros.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I agree we should not see any #ifdef platform anywhere in the code but in specific .cpp files. It's mostly in shaderCompilerCommon.cpp so I'll let you do the change when you are done with your shader compilation work.

uint16_t height = static_cast<uint16_t>(info[2].As<Napi::Number>().Uint32Value());
bgfx::FrameBufferHandle frameBufferHandle{};

// This is WIP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there an issue to track?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes #398

0.f, 0.f, 1.f, 0.f,
0.f, 0.f, 0.f, 1.f };
bx::mtxMul(tmpMatrix, value.Data.data(), flipMatrix);
bgfx::setUniform({ it.first }, tmpMatrix, value.ElementLength);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: clang format this code

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

@CedricGuillemet
CedricGuillemet merged commit 02c3474 into BabylonJS:master Sep 29, 2020
CedricGuillemet added a commit to CedricGuillemet/BabylonNative that referenced this pull request Sep 2, 2026
080f466590 Added bx::hasUniqueObjectRepresentation type trait. (BabylonJS#420)
671a001d3e tinystl: reset() keeps the bucket table; clear() still shrinks to 9. (BabylonJS#419)
9f874936e2 Added ability to move file into trash. (BabylonJS#418)
9e3fadf6f1 Fixed floatFlip, SIMD variable-shift masks and %g precision, and added tests. (BabylonJS#417)
26ff6d907d MinGW: Fix sal. (BabylonJS#416)
0a09590ac8 Added positional arguments to CommandLine. (BabylonJS#415)
822be40688 Scanner: Class::Identifier must not start with a digit. (BabylonJS#414)
43bfe29402 Fixed DLL search order hijacking in dlopen. (BabylonJS#413)
0b001f5f36 MSVC: Added ARM64 target. (BabylonJS#412)
056ea3486c Scanner: Cleanup. (BabylonJS#411)
9916e720fc Added scanner. (BabylonJS#410)
cd6720ce9a platform.h: detect MSVC ARM64 as 64-bit and raise 32-bit Windows minimum (BabylonJS#409)
5a2b876258 fromString: Parse hex float literals. (BabylonJS#408)
a7257985fa Added bx::isEqual for bx::StringView. (BabylonJS#407)
c98e98cde1 Fixed amalgamated build. (BabylonJS#406)
ed183eb246 Allow files with 10k+ lines to align nicely. (BabylonJS#405)

git-subtree-dir: Dependencies/bx
git-subtree-split: 080f466590c161664f2a3c5d21e05bd31cc9c8c8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support render target textures

2 participants