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

Rewrite ProgramConfiguration #4934

Merged
merged 1 commit into from
Jul 6, 2017
Merged

Conversation

jfirebaugh
Copy link
Contributor

@jfirebaugh jfirebaugh commented Jul 3, 2017

benchmark master 83fb52d rewrite-program-configuration 96e5fb8
map-load 99 ms 83 ms
style-load 67 ms 60 ms
buffer 1,002 ms 945 ms
fps 60 fps 60 fps
frame-duration 5.1 ms, 0% > 16ms 5.4 ms, 0% > 16ms
query-point 0.78 ms 0.87 ms
query-box 50.73 ms 50.52 ms
geojson-setdata-small 4 ms 4 ms
geojson-setdata-large 129 ms 130 ms

Launch Checklist

  • briefly describe the changes in this PR
  • post benchmark scores
  • manually test the debug page

@jfirebaugh jfirebaugh requested a review from anandthakker July 3, 2017 20:55
@mourner
Copy link
Member

mourner commented Jul 3, 2017

Awesome! Do you know if there's a performance impact from the float type change? Would it cause more data to be uploaded?

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

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

🍏 (notwithstanding two minor suggestions)

@@ -76,6 +35,162 @@ export type Program = {
[string]: any
}

function packColor(color: [number, number, number, number]): Array<number> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the return type here should be [number, number].


program.fragmentSource = program.fragmentSource.replace(re, (match, operation, precision, type, name) => {
fragmentPragmas[name] = true;
return operation === 'define' ? `
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd favor the more explicit if (operation === 'define') ... else if (operation === 'initialize') (or the equivalent switch) here and below, so as to (re-)articulate how our custom #pragma syntax works.

@jfirebaugh jfirebaugh force-pushed the type-data branch 3 times, most recently from e71e771 to 18d5442 Compare July 5, 2017 16:28
@jfirebaugh jfirebaugh force-pushed the rewrite-program-configuration branch from 94914ce to f3bb3b3 Compare July 5, 2017 16:43
@jfirebaugh jfirebaugh changed the base branch from type-data to master July 5, 2017 16:43
@jfirebaugh jfirebaugh force-pushed the rewrite-program-configuration branch from f3bb3b3 to 8e61d8b Compare July 5, 2017 18:13
@jfirebaugh
Copy link
Contributor Author

jfirebaugh commented Jul 5, 2017

Do you know if there's a performance impact from the float type change? Would it cause more data to be uploaded?

Yes, in some cases -- I believe the majority -- the buffers for data-driven properties will be larger. For source functions, non-color data-driven properties that were previously typed as Uint8 will use 4x space; Uint16 and color properties, 2x. For composite functions, it depends on how many stops the function uses and the type of the property. Color properties will generally use less space. Functions with four or more stops will generally use less space. Other composite functions will generally use the same amount of space or more space. The space for non-data-driven property values is unchanged.

Overall, I expect the space increase to have a minor impact, and I think it's worth it to simplify the implementation, make gl-js consistent with gl-native, and eliminate bugs like #3954 and #4438 where using a 16-bit integer results in truncation and limited range.

* Expand #pragmas to #ifndefs early on. This is written in such a way that it can be reused by gl-native's generate-shaders.js.
* Binder interface for paint properties, with a concrete implementation for each binding strategy.
* Always use floats for paint property attributes, without a multiplication factor. (Fixes #4438)
* When binding composite functions, interpolate between two stop values only: zoom and zoom + 1.
@jfirebaugh jfirebaugh force-pushed the rewrite-program-configuration branch from 8e61d8b to a3b43f6 Compare July 5, 2017 18:34
@jfirebaugh jfirebaugh merged commit e93db1b into master Jul 6, 2017
@jfirebaugh jfirebaugh deleted the rewrite-program-configuration branch July 6, 2017 17:37
jfirebaugh added a commit that referenced this pull request Sep 18, 2017
The previous value was due to using a 16-bit unsigned integer for the attribute. In #4934, we switched to floats, removing the limitation.
jfirebaugh added a commit that referenced this pull request Sep 18, 2017
The previous value was due to using a 16-bit unsigned integer for the attribute. In #4934, we switched to floats, removing the limitation.
jfirebaugh added a commit that referenced this pull request Sep 18, 2017
The previous value was due to using a 16-bit unsigned integer for the attribute. In #4934, we switched to floats, removing the limitation.
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