Skip to content
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

Clean up root #1001

Merged
merged 2 commits into from Mar 27, 2019

Conversation

Projects
None yet
5 participants
@Pessimistress
Copy link
Contributor

Pessimistress commented Mar 22, 2019

No description provided.

Pessimistress added some commits Mar 22, 2019

@Pessimistress Pessimistress requested review from ibgreen and georgios-uber Mar 22, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 22, 2019

Coverage Status

Coverage decreased (-0.03%) to 46.191% when pulling b748e10 on eslint-module into 0859f92 on master.

@ibgreen
Copy link
Contributor

ibgreen left a comment

The JSON WebGL API description files were intended for seer integration, since that is no longer something we are working on I'm OK with them being removed.

@@ -59,7 +59,7 @@
"babel-plugin-version-inline": "^1.0.0",
"coveralls": "^2.13.0",
"eslint-plugin-import": "^2.16.0",
"eslint-plugin-luma-gl-custom-rules": "file:./eslint",
"eslint-plugin-luma-gl-custom-rules": "file:./dev-modules/eslint-plugin-luma-gl-custom-rules",

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 22, 2019

Contributor

Nit: I assume we not planning to publish this module? If we did it would either be part of probe or ocular? In that case I am not sure dev-modules is the right place.

Although I guess we could name it something like @probe.gl/eslint so that we publish it on npm under the right name now, then we can move the source later?

@tsherif

This comment has been minimized.

Copy link
Member

tsherif commented Mar 22, 2019

@ibgreen

The JSON WebGL API description files were intended for seer integration, since that is no longer something we are working on I'm OK with them being removed.

If that's the case, can we remove the timers from Model and BaseModel? I'm concerned that if used they'll interfere with the frame timers.

@ibgreen

This comment has been minimized.

Copy link
Contributor

ibgreen commented Mar 22, 2019

If that's the case, can we remove the timers from Model and BaseModel? I'm concerned that if used they'll interfere with the frame timers.

Well, Seer integration should still be working, it is not a feature we have so far discussed discontinuing. I was mainly commenting on the fact that certain in-progress development of seer stopped when the original author moved on, so no reason to keep in-progress work alive in the source tree.

I have found it useful to see individual results directly in seer for each model, it provides a lot more information than a frame number when multiple things are being rendered. It has seemed to work in the past, but perhaps there are limitations, and then we'll have to understand whether fixing those is worth the cost.

that if used they'll interfere with the frame timers.

When are they currently used? Are they only activated when the seer Chrome extension is opened? If that is the case we can do some testing and check how it looks?


About Seer: One of the potential integration points I have envisioned is to have an automatic seer integration with the stats objects you have been working on, such that we don't need to inject stats widgets into every app to instrument it, instead just open Chrome dev tools and get the data right there!

@ibgreen

This comment has been minimized.

Copy link
Contributor

ibgreen commented Mar 22, 2019

To add to previous comment: Ideally all performance collection should go through a single system (stats object).

so just like the existing resource counter setup was an integration target, this per-model measurement should ideally also go through the luma stats object, and seer should read it from there.

That should at least ensure that we don't have competing systems doing the same thing, but a single design where we can more easily evaluate what is possible/practical.

@ibgreen ibgreen merged commit 37592f4 into master Mar 27, 2019

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 46.191%
Details
license/cla Contributor License Agreement is signed.
Details

@ibgreen ibgreen deleted the eslint-module branch Mar 27, 2019

ibgreen added a commit that referenced this pull request Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.