Skip to content

Revert shader support for unquantized vertex tables #3401

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

Merged
merged 11 commits into from
Mar 24, 2022

Conversation

DStradley
Copy link
Contributor

@DStradley DStradley commented Mar 22, 2022

The PR for Support unquantized vertex tables #3288 caused GPU slowdowns on performance testing, particularly on slower GPUs. After a lot of investigation and optimizing GPU code, it was found that even after optimization, there was still enough difference to justify splitting the unquantized support into separate shaders, which will be done in a separate PR.

This PR removes support for unquantized vertex tables from the current shaders, and turns off the flag to use unquantized. It does keep an optimized version of shader code for pre-loading the vertex tables, that was also introduced in 3288.

Also learned during this investigation:

  • Indexing a vector (rather than accessing its components, at least in cases where it can't be directly replaced by the component version) causes a performance hit and causes zingers on at least some Intel Integrated graphics. An example of this is declaring a "vec4 v", then using it inside a loop like "v[i] = func ();" where i is expected to address x, y, z, w components of the vector sequentially.
  • Using a global array (which at assembly level uses an indexable temp register instead of a regular temp register) is a performance hit over separately declared vars. E.g.: using "vec4 g_temp0; vec4 g_temp1; vec4 g_temp2" is better than using "vec4 g_temp[3];"

@pmconne
Copy link
Member

pmconne commented Mar 23, 2022

@DStradley I am looking into the decoration issue.

Copy link
Member

@pmconne pmconne left a comment

Choose a reason for hiding this comment

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

3 failing tests. UPDATE: They're all passing now.

  1) GraphicBuilder
       createMesh
         should preserve polyface normals:

      AssertionError: expected 3 to equal 5
      + expected - actual

      -3
      +5

      at verifyParams (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:170:53)
      at System.IModelApp_1.IModelApp.renderSystem.createMesh (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:157:11)
      at System.createTriMesh (d:\js\s\imodeljs\core\frontend\src\render\RenderSystem.ts:362:17)
      at Mesh.getGraphics (d:\js\s\imodeljs\core\frontend\src\render\primitives\mesh\MeshPrimitives.ts:301:21)
      at GeometryAccumulator.saveToGraphicList (d:\js\s\imodeljs\core\frontend\src\render\primitives\geometry\GeometryAccumulator.ts:207:28)
      at PrimitiveBuilder.finishGraphic (d:\js\s\imodeljs\core\frontend\src\render\primitives\geometry\GeometryListBuilder.ts:168:22)
      at PrimitiveBuilder.finish (d:\js\s\imodeljs\core\frontend\src\render\primitives\geometry\GeometryListBuilder.ts:47:26)
      at test (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:198:30)
      at Context.call (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:203:7)
      at callFn (@/lib/runnable.js:366:21)

  2) GraphicBuilder
       createMesh
         should generate normals for shapes if requested:

      AssertionError: expected 3 to equal 5
      + expected - actual

      -3
      +5

      at verifyParams (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:170:53)
      at System.IModelApp_1.IModelApp.renderSystem.createMesh (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:157:11)
      at System.createTriMesh (d:\js\s\imodeljs\core\frontend\src\render\RenderSystem.ts:362:17)
      at Mesh.getGraphics (d:\js\s\imodeljs\core\frontend\src\render\primitives\mesh\MeshPrimitives.ts:301:21)
      at GeometryAccumulator.saveToGraphicList (d:\js\s\imodeljs\core\frontend\src\render\primitives\geometry\GeometryAccumulator.ts:207:28)
      at PrimitiveBuilder.finishGraphic (d:\js\s\imodeljs\core\frontend\src\render\primitives\geometry\GeometryListBuilder.ts:168:22)
      at PrimitiveBuilder.finish (d:\js\s\imodeljs\core\frontend\src\render\primitives\geometry\GeometryListBuilder.ts:47:26)
      at test (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:216:28)
      at Context.call (d:\js\s\imodeljs\core\frontend\src\test\render\GraphicBuilder.test.ts:221:7)
      at callFn (@/lib/runnable.js:366:21)

  3) VertexLUT
       should produce correct VertexLUT.Params from unquantized MeshArgs:

      AssertionError: expected +0 to equal 128
      + expected - actual

      -0
      +128

      at expectMeshParams (d:\js\s\imodeljs\core\frontend\src\test\render\primitives\VertexTable.test.ts:20:36)
      at Context.call (d:\js\s\imodeljs\core\frontend\src\test\render\primitives\VertexTable.test.ts:185:5)
      at callFn (@/lib/runnable.js:366:21)
      at Test.run (@/lib/runnable.js:354:5)
      at Runner.runTest (@/lib/runner.js:677:10)
      at fn (@/lib/runner.js:801:12)
      at next (@/lib/runner.js:594:14)
      at fn (@/lib/runner.js:604:7)
      at next (@/lib/runner.js:486:14)
      at http://localhost:3000/@/lib/runner.js:572:5

@pmconne pmconne marked this pull request as ready for review March 24, 2022 12:05
@pmconne pmconne requested review from kabentley, bbastings and a team as code owners March 24, 2022 12:05
@DStradley DStradley merged commit 71ba31e into master Mar 24, 2022
@DStradley DStradley deleted the opt-shdr-ba6-noarray-8-opt2-quantonly branch March 24, 2022 20:30
DStradley added a commit that referenced this pull request Mar 24, 2022
pmconne added a commit that referenced this pull request Mar 29, 2022
* Always enable the CloudStorageCacheChannel (#3384)

* Always enable the CloudStorageCacheChannel.

We recently discovered a race condition in `TileAdmin` that causes some tiles to be re-generated despite already existing in the cloud storage tile cache.  Basically, it was possible for some tiles to start loading before `TileAdmin.initializeRpc()` had finished lazily enabling the `CloudStorageCacheChannel`.

After taking a closer look at `CloudStorageTileCache`, I believe we should not need to have a separate `isUsingExternalTileCache` RPC method or even make the `CloudStorageCacheChannel` optional at all, because we can just use `getTileCacheContainerUrl` to determine whether caching is supported by the backend. `CloudStorageTileCache` will **not** actually make any requests to the tile cache if `getTileCacheContainerUrl` returns an "empty" `CloudStorageContainerUrl` object, nor will it ever re-request that container URL if it comes back empty.

I've added tests that cover this `CloudStorageTileCache ` behavior, and also fixed up its error handling too.

* fix lint.

* Make IModelTileRequestChannels non-nullable and fix tests.

* remove deferral from transformer (#3219)

* replace any cast with type safe protected member access

* add local test of iModel with missing nav prop ids in target

* fix local test

* add automatic impl of collectPredecessorIds for generated js classes

* add tests to class registry for new generated class behavior

* replace local snapshot loading test with hand crafted reproduction

* add tests for isProperSubclassOf

* fix linter rule override location

* rush change

* rush extract-api

* fix bad intellisense recommending old package name

* add ClassUtils doc group description

* fix typing of isProperSubclass and add typing tests

* rush extract-api

* fix casting undefined causing test failure, dont cast just use the widened type

* add another local test of full content

* add almost deep equal testing and first iteration of fully tested target content

* WIP replacing predecessors with partially committed elements

* complete initial draft of element finalization

* removed all usage of defering

* add start of proxy that will translate biscore classes with convenience renames of props in the js impl

* remove skipElement/processDeferredElements references

* remove class utils which aren't being used in the newer impl anymore

* revert class registry import changes

* wip handling of predecessors from manual classes

* make all handler methods optionally async

* final version of deepkey before removal

* experimenting with new requiredReferenceKeys format

* basic transform completing

* make new async behavior in IModelTransformer.onExportElement not a breaking change

* reintroduce skipElement since it was public and dummy calls to processDeferred

* warn on unresolved references, filter missing predecessors which prevented onComplete from running

* Revert "revert class registry import changes"

This reverts commit 2da27cc.

* Revert "remove class utils which aren't being used in the newer impl anymore"

This reverts commit 0fc971a.

* revert reversion of ClassRegistry default collectPredecessors for generated classes

* remove no longer used isGeneratedClassTag checker

* fix pendingReference completion update

* minor cleanup of new code

* fix false-positive deprecation warning where falsiness is not handled

* fixes for catalog transformation behavior

* implement more map methods in Uint32Map

added keys, values, entries, delete, and has

* use Uint32Map in the transformer for Id64String maps

* clean up pair map logic of pendingReferences map

* add iterator and value iteration to UInt32Set

* WIP linkTableReferences cache usage

* NOTE COMMIT - I seem to have encountered a bug

after inserting a model selector with model props, it is immediately
viewable. However, upon the corresponding aspect insert (really any
insert), the models property is no longer visible... I assume it has
something to do with caching and the element being in memory during the
previous insert. Either way it is a stopper on this work for now

* add note about future support for nav properties on rel instances

* add new pending reference map

* snapshot attempt at exporting models before element depending upon them

* basic new pendingReferences map usage with element vs model reference distinction

* fix pending references over elements without exported submodels

* experimental cleaner pending reference finalizer building from predecessors

* simplify pendingReferences logic

* make local test use public iModel with alignments and geometry props

* temp work, need to context switch

* use integrated terminal in transformer tests

* handle out of order code transformations

* fix test which was using source ids to find target elements

* reinstate behavior for dangling predecessors

* added 'invalidate' dangling predecessor behavior

* refactor reference readiness which fixed everything

* better names for readiness concept

* remove lodash.get since no deep keys are necessary

* merge: Create predecessors even in generated js classes for the transformer, and handle deferred element relationships (#3115)

- consider navigation properties to be predecessors in non-core classes
  - add a skipped test for cyclical relationships which are valid but the transformer cannot handle currently. There is ongoing work to address this and that test can be unskipped then
- process relationships even if their elements were deferred
- added more exhaustive tests of data integrity when transforming iModels
  - these tests test a bug that was fixed in a native PR
- add a utility for checking if a class is a subclass of another. and if a JS Entity class is a subclass of another

Co-authored-by: Michael Belousov <[email protected]>

* fix bad infinite recursion bug

* fix unsaved source not showing source elements

* remove lodash.get from lockfile

* fix super calling without a hack

* fix super call collect predecessor ids

* experimental better class registry subclass test

* fix merged change in master which is no longer necessary

* clean up string comprehension

* dont change line endings of browser-approved-packages file

* fix lint issues in transformer package

* rush extract-api

* remove out of date note about what to test next

* force out of order export in test without using deferral's skip element mechanism

* remove requiredReferenceKeys since we still use predecessors and it forced breaking async changes that weren't necessary

* rush extract-api

* revert unnecessary backend changes

* redo rush change

* remove old version of ClassUtils change

* revert unnecessary change to core-bentley

* undo bad merge; use a model containing non-core schema (road rail) for the exhaustive identity transform test

* Revert "revert unnecessary backend changes"

This reverts commit 498b20a.

* Revert "remove requiredReferenceKeys since we still use predecessors and it forced breaking async changes that weren't necessary"

This reverts commit af00708.

* refactor construction of ElementProcessState objects and add async workaround to prevent breaking changes

* fix async order of preExportTask and refactor ElementProcessState names

* allow exhaustive transform check to ignore non-default-transformed classes, and handle more expected transformed display style content

* fix asset file resolution

* fix incorrect handling of provenance aspect class exclusion

* revert optional async handler responses

fix bad HandlerResponse import

* rush lint

* rush extract-api

* cleanup requiredReferenceKeys accessibility

* remove preExportTask hack in favor of a new internal handler method that runs before onExportElement

* revert promise-returning changes to test IModelExportHandlers

* clean up doc comments in changes, trim mapId64 to far less container types

trimming to less container types since we only need to cover simple ones
for "requiredReferenceKeys" where it is used

* more doc comment fixes

* revert eslint rule changes

* update rush change

* add note about how onTransformElement should not have side-effects

* remove transformer specific note in IModelCloneContext

* (doc) comment tweaks

* rush extract-api

* remove unnecessary requiredReferenceKeys impl from Model

* add docs-desc-group to new PendingReferenceMap file

* rewrite NextVersion entry

* remove nextversion detail about onTransformElement needing to be side-effect free, this is not new

* change make name to makeOnCompleteCallback

* remove usage of Uint32 map since its optimizations dont apply to heavy string usage like the transformer

* replace PendingReferenceProps+class with just PendingReference and a namespace for utility functions

* collapse what used to be a long type union

* use assert for better check

* note deprecations and change in behavior of newly deprecated functions

* remove rush change of core-bentley

* remove alignments.bim test, going to use it for performance tests instead

* rush extract-api

* remove no longer accurate notes and add deprecation message

* highlight that deprecated APIs are beta

Co-authored-by: Michael Belousov <[email protected]>

* 3.2.0-dev.22

* Fix namespace imports for esm (#3383)

* fix imports to support esm

* Enable manaul run for the extract api yaml (#3411)

* Revert shader support for unquantized vertex tables (#3401)

* noarray-8-opt2 based on 3288

* ba1-noarray-8-opt2 with unquant removed

* no readMaterialAtlas for instanced

* Cleanup

* Fix transform for quantized vertices.

* document unquantized positions don't display properly.

* adjust expected num rgba per vertex.

* Adjust test for vertex table transposition.

* Update opt-shdr-ba6-noarray-8-opt2-quantonly_2022-03-23-21-17.json

* cleanup.

Co-authored-by: Paul Connelly <[email protected]>

* Add splitter to allow panel widget resizing (#3367)

* basic functionality working

* remove unused hook

* set min floating widget size

* update to use position absolute splitter and new unpin icon

* Update to only render two vertical panel sections

* add ability to save restore widget panel splitter location

* Add ability to specify defaultFloatingSize for widgets that don't have intrinsic size

* updates to fix drop targets for widgets

* Fix missing parameter in provide widgets call. Also pass provider to useStage callback.

* Add useSpecificWidgetDef for use by itemproviders

* Update to better handle case were index was used to locate wrong panel section

* fix debub by adding jsdom to package and working on test

* update unit test to reflect current UX design

* extract-api and test update

* Updates to NextVersion to add entry for Widget Panel Changes.

* fix doc links

* extract-api

* Bump internal NineZoneState version number since Center widget section is no longer supported.

* Fix App icon size in UI1 mode

* Have FrontstageComposer updateWidgetDefs before initial render.

* Fix typo

* fix lint errors

* rush change

* Widget documentation updates

* try rush update to get past security warnings

* UI: Support react icons throughout AppUi (#3412)

* Support icons

* Add utility method for getting abstract
props for React icons.
Use itwinui-icons-react in the test provider

* Update WidgetDef unit tests and BacklstageItem
snaps.

* More unit testing for WidgetDef
Add unit test for getAbstractPropsForReactIcon()

* Rush change

* NextVersion update & rush lint

* extract-api and rush lint

* Update packages

* Extract-api after merge with master

* fix missing newline.

* Extract-api

Co-authored-by: Paul Connelly <[email protected]>

* BisSchemas loader performance tests (#3402)

* Add BisSchemas loader performance tests

* 3.2.0-dev.23

* Presentation: Final docs review (#3388)

* Cross-link rules & specifications

* Add attribute information tables

* fixup "Cross-link rules & specifications"

* Unify header casing

* Unify example tables

* Unify TS and MD docs

* Stop using and deprecate `ConditionContainer`

* Make all presentation doc sub-folders lowercase

* Remove doc image source `.snag` files

* Fix broken links

* rush change

* Fix broken links

* Update presentation/common/src/presentation-common/rules/customization/SortingRule.ts

Co-authored-by: Robert Lukasonok <[email protected]>

* Get rid of performance notes in attribute tables

* Update ruleset schema

Co-authored-by: Robert Lukasonok <[email protected]>

* Update error handling in ECSchemaRpcImpl and frontend logging (#3413)

* update schema rpc error handling

* change log

* revert pnpm-lock

Co-authored-by: Paul Connelly <[email protected]>

* @bentley/imodeljs-native 3.2.2

* merge cleanup; extract-api

* wtf does this code keep changing.

* .'

* changelog

* synchronize tile format version.

* extract-api

* 3.0.3 changelogs (#3422)

Co-authored-by: imodeljs-admin <[email protected]>

* 3.2.0-dev.24

* 3.2.0-dev.25

* 3.2.0-dev.26

* @bentley/imodeljs-native 3.2.3

* fix broken doc link

Co-authored-by: Bill Goehrig <[email protected]>
Co-authored-by: Michael Belousov <[email protected]>
Co-authored-by: Michael Belousov <[email protected]>
Co-authored-by: imodeljs-admin <[email protected]>
Co-authored-by: kckst8 <[email protected]>
Co-authored-by: Colin Kerr <[email protected]>
Co-authored-by: DStradley <[email protected]>
Co-authored-by: Bill Steinbock <[email protected]>
Co-authored-by: NancyMcCallB <[email protected]>
Co-authored-by: Akshay Nangare <[email protected]>
Co-authored-by: Grigas <[email protected]>
Co-authored-by: Robert Lukasonok <[email protected]>
Co-authored-by: christophermlawson <[email protected]>
Co-authored-by: Seamus Kirby <[email protected]>
Co-authored-by: nick.tessier <[email protected]>
Co-authored-by: Caleb Shafer <[email protected]>
Co-authored-by: Michel D'Astous <[email protected]>
Co-authored-by: MarcBedard8 <[email protected]>
Co-authored-by: Arun George <[email protected]>
Co-authored-by: Arun George <[email protected]>
Co-authored-by: GerardasB <[email protected]>
Co-authored-by: Spencer Barnes <[email protected]>
Co-authored-by: dassaf4 <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: williamkbentley <[email protected]>
Co-authored-by: Saulius Skliutas <[email protected]>
Co-authored-by: MarcNeely <[email protected]>
Co-authored-by: Paulius Valiūnas <[email protected]>
Co-authored-by: bbastings <[email protected]>
Co-authored-by: markschlosseratbentley <[email protected]>
Co-authored-by: AJMigliore <[email protected]>
Co-authored-by: Travis Cobbs <[email protected]>
Co-authored-by: mgooding <[email protected]>
Co-authored-by: Alina Paliulionytė <[email protected]>
Co-authored-by: Alina Paliulionyte <[email protected]>
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.

3 participants