Skip to content

Conversation

@antoinerg
Copy link
Contributor

Closes #4124

@etpinard etpinard added status: reviewable bug something broken labels Aug 13, 2019
@etpinard
Copy link
Contributor

Excellent 💃

@antoinerg antoinerg merged commit 11c9879 into master Aug 13, 2019
@antoinerg antoinerg deleted the fix-4124 branch August 13, 2019 21:17
for(i = optsOut.text.length; i < count; i++) {
optsOut.text[i] = '';
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ this is mutating the original data array - I don't think we want to do that.

Screen Shot 2019-08-13 at 5 29 39 PM

Copy link
Contributor Author

@antoinerg antoinerg Aug 13, 2019

Choose a reason for hiding this comment

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

Nice catch @alexcjohnson !

Because this file conflicts with my texttemplate branch, I will have to modify it anyway. How should I solve this problem? Can I simply slice it or is this a no go for performance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say slice is fine - as long as you only do it when text.length < count. I think it's totally reasonable to say "for best performance make sure your arrays are all the same length" and prioritize stability over edge-case performance. The presumably higher-perf alternative would be to track down where the array is being indexed and fall back to a blank string at that point... but looks like that's deep in a regl submodule so that sounds painful 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not a big deal. But since we're planning on releasing 1.49.3 and this PR got merged, @antoinerg can you make another PR off master to fix and test the optsOut.text mutation?

Copy link
Contributor

Choose a reason for hiding this comment

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

.... instead of fixing it in #4071

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard Done in #4126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug something broken

Projects

None yet

Development

Successfully merging this pull request may close these issues.

scattergl: text arrays shorter than data arrays throws error

3 participants