Skip to content

QNTM-4394 Match Revit CEF intitializtion settings#9058

Merged
saintentropy merged 3 commits intoDynamoDS:masterfrom
saintentropy:QNTM-4934
Aug 27, 2018
Merged

QNTM-4394 Match Revit CEF intitializtion settings#9058
saintentropy merged 3 commits intoDynamoDS:masterfrom
saintentropy:QNTM-4934

Conversation

@saintentropy
Copy link
Contributor

@saintentropy saintentropy commented Aug 19, 2018

Purpose

The purpose of this PR is to match Dynamo’s CEF initialization settings with those set by Revit. Currently the delta is that Dynamo disables GPU acceleration. For Revit 2018 and 2019, Revit does not disable GPU acceleration in the CEF context as there are supported Revit addins who require LMV (and threejs). When Dynamo is run in these contexts, Revit’s CEF settings are the first to be set and Dynamo’s preferences (and other subsequent CEF addins) are ignored. Although not every Dynamo session (Sandbox for example) has this situation, disabling GPU rendering in core has the potential to mask bugs that are present for a majority of users. Dynamo core should match Revit’s CEF settings and version and should continue to match settings and versions as they change in future releases.

QNTM-4934

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @alfarok

FYIs

@sm6srw @Dewb

@sm6srw
Copy link
Contributor

sm6srw commented Aug 20, 2018

We are also initializing CEF here:

settings.SetOffScreenRenderingBestPerformanceArgs();

I don't know if that ever happens but it should follow the same pattern.

@saintentropy
Copy link
Contributor Author

Ahh good catch!

@alfarok
Copy link
Contributor

alfarok commented Aug 20, 2018

@saintentropy I am still curious why I can't get three.js to launch in D4R 2018. This was the check I was using for WebGL

if (webglAvailable()) {
    renderer = new THREE.WebGLRenderer( { antialias: false, alpha: true } );
} else {
    renderer = new THREE.CanvasRenderer();
    defaultShadedMaterial = new THREE.MeshBasicMaterial({ color: 0x00ff00 });
}

function webglAvailable() {
try {
    var canvas = document.createElement('canvas');
    return !!(window.WebGLRenderingContext && (
        canvas.getContext('webgl') ||
        canvas.getContext('experimental-webgl'))
    );
} catch (e) {
    return false;
}

Anyway, this LGTM. We should keep a close eye on conflicts between multiple CEF reliant apps, especially librarie.js. I also came across this line which is slightly different in package manager UI (which isn't implemented) but it seems the Singapore team consistently disabled the GPU when using CEF.

@mjkkirschner
Copy link
Member

  • Have you verified that as a revit addin you can start a webgl enabled cef window?
  • noticed any issues with library after this change?

@saintentropy
Copy link
Contributor Author

@mjkkirschner 1) Yes we have verified that and 2) no we have noticed specific issues with Library although there are still intermittent issues with Library that seem to occur independent of this settings change. Specifically, the notes in the older commits were about flickering. That has not be observed in our use... perhaps CEF version changes since then have improved that defect. Regardless, whatever we observe with this change is what is currently observed by users in Revit 2018, 2019, 2020.

@saintentropy
Copy link
Contributor Author

saintentropy commented Aug 24, 2018

All Self service tests appears to pass except 1 that is also failing on master

@alfarok
Copy link
Contributor

alfarok commented Aug 27, 2018

@saintentropy just pinged @smangarole on this. I believe some recent changes by Luke may be causing failures so we are working to get that sorted out

@saintentropy
Copy link
Contributor Author

Ok thanks @alfarok I will go ahead and merge and wait for the test to be resolved.

@saintentropy saintentropy merged commit 8e435ea into DynamoDS:master Aug 27, 2018
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.

4 participants