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
Merged

Clean up root #1001

merged 2 commits into from
Mar 27, 2019

Conversation

Pessimistress
Copy link
Collaborator

No description provided.

@coveralls
Copy link

Coverage Status

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

Copy link
Collaborator

@ibgreen ibgreen left a comment

Choose a reason for hiding this comment

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

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",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Collaborator

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
@ibgreen ibgreen deleted the eslint-module branch March 27, 2019 22:33
ibgreen pushed 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants