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

Changed reorder()'s sort to be stable #110

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@KPWhiver
Contributor

KPWhiver commented Jun 8, 2015

The Array.sort() function in reorder() can be unstable depending on the
browser. This means that if some of the values passed in to the
reorder() function result in having the same pixel height, their
respective dimensions might be ordered differently everytime reorder()
is called. To solve this issue, the dimensions are ordered on name
whenever such a conflict occurs.

Changed reorder()'s sort to be stable
The Array.sort() function in reorder() can be unstable depending on the
browser. This means that if some of the values passed in to the
reorder() function result in having the same pixel height, their
respective dimensions might be ordered differently everytime reorder()
is called. To solve this issue, the dimensions are ordered on name
whenever such a conflict occurs.

@bbroeksema bbroeksema self-assigned this Jun 8, 2015

Added a brushState function to all brushmodes
This can help dissolve the ambiguous situation where __.brushed.length
=== __.data.length. This can either mean that no brushes are active
(nothing is brushed) or that everything is brushed.
@KPWhiver

This comment has been minimized.

Show comment
Hide comment
@KPWhiver

KPWhiver Jun 18, 2015

Contributor

Two more commits were added to my fork. They concern a new feature that allows you to render the brushed items with a different color, instead of removing the non brushed items.

Contributor

KPWhiver commented Jun 18, 2015

Two more commits were added to my fork. They concern a new feature that allows you to render the brushed items with a different color, instead of removing the non brushed items.

@syntagmatic

This comment has been minimized.

Show comment
Hide comment
@syntagmatic

syntagmatic Jun 19, 2015

Owner

I think KPWhiver@ee63985 should be merged at any time.

KPWhiver@09b3fb5 also looks good, enabling smarter brushes and exposing the hidden function brushExtents

KPWhiver@6bc150d brings up, for me, some uncomfortable issues about the rendering API. It's quite configuration-oriented and relies on numerous boolean tests in the core code. d3.parcoords strongly assumes a 2d <canvas> context. Actually an SVG context would be more useful for expert d3 users (or print publications) while Julian's WebGL context would be more useful as a high-performance context.

I'm on the fence about tagBrushed in the third commit. This modifies an attribute on the dataset. Does it enable the brushes to do less bookkeeping? Or is it extra bookkeeping?

Thanks Klaas for the contribution! I'll leave it to @bbroeksema to decide what to merge

Owner

syntagmatic commented Jun 19, 2015

I think KPWhiver@ee63985 should be merged at any time.

KPWhiver@09b3fb5 also looks good, enabling smarter brushes and exposing the hidden function brushExtents

KPWhiver@6bc150d brings up, for me, some uncomfortable issues about the rendering API. It's quite configuration-oriented and relies on numerous boolean tests in the core code. d3.parcoords strongly assumes a 2d <canvas> context. Actually an SVG context would be more useful for expert d3 users (or print publications) while Julian's WebGL context would be more useful as a high-performance context.

I'm on the fence about tagBrushed in the third commit. This modifies an attribute on the dataset. Does it enable the brushes to do less bookkeeping? Or is it extra bookkeeping?

Thanks Klaas for the contribution! I'll leave it to @bbroeksema to decide what to merge

@KPWhiver

This comment has been minimized.

Show comment
Hide comment
@KPWhiver

KPWhiver Jun 19, 2015

Contributor

I have changed the way the brushed items are rendered. There is now a new canvas layer called brushed which is between foreground and highlight. On this canvas layer the brushed items are rendered. The brushed attribute is no longer needed, so the data is not modified.

Contributor

KPWhiver commented Jun 19, 2015

I have changed the way the brushed items are rendered. There is now a new canvas layer called brushed which is between foreground and highlight. On this canvas layer the brushed items are rendered. The brushed attribute is no longer needed, so the data is not modified.

Added the brushedColor and alphaOnBrushed options
brushedColor() works the same way as color() but it will only color the
brushed items. When brushedColor is set to null (default) it will use
color instead.
alphaOnBrushed() sets the alpha of the data items when brushing takes
place. This can be used to render both data and brushed at the same time
but putting extra emphasize on the brushed items.

To achieve this a new canvas layer was added for the brushed items.
Thanks to this new layer it is now possible to rerender the brushed
items independently from the rest of the data. This can be done with the
renderBrushed function.
@KPWhiver

This comment has been minimized.

Show comment
Hide comment
@KPWhiver

KPWhiver Jun 24, 2015

Contributor

@syntagmatic: Based on some feedback from @bbroeksema I have changed the last commit a bit. Instead of a brushedRenderMode there are now two functions with which the same (and more) can be achieved:
-brushedColor behaves like color, but only for the items on the brushed canvas layer (it defaults to using color)
-alphaOnBrushed fades the foreground layer when brushing takes place (it defaults to 0)

In principle these functions make the current shadows canvas layer obsolete, and it could be removed. The current functionality is more flexible and also faster since it uses renderqueue to render the brusheddata.

Contributor

KPWhiver commented Jun 24, 2015

@syntagmatic: Based on some feedback from @bbroeksema I have changed the last commit a bit. Instead of a brushedRenderMode there are now two functions with which the same (and more) can be achieved:
-brushedColor behaves like color, but only for the items on the brushed canvas layer (it defaults to using color)
-alphaOnBrushed fades the foreground layer when brushing takes place (it defaults to 0)

In principle these functions make the current shadows canvas layer obsolete, and it could be removed. The current functionality is more flexible and also faster since it uses renderqueue to render the brusheddata.

@bbroeksema

This comment has been minimized.

Show comment
Hide comment
@bbroeksema

bbroeksema Jun 30, 2015

Collaborator

@syntagmatic I merged all changes from Klaas and did some additional work to update the the shadows functionality, update the documentation and do a version bump.

For your reference:

If you still have objections against c4f6718, please let me know and I'll make an effort to address the issues. However, I think @KPWhiver did a good job and we have good results in our application with this new functionality. For now closing the pull request.

Collaborator

bbroeksema commented Jun 30, 2015

@syntagmatic I merged all changes from Klaas and did some additional work to update the the shadows functionality, update the documentation and do a version bump.

For your reference:

If you still have objections against c4f6718, please let me know and I'll make an effort to address the issues. However, I think @KPWhiver did a good job and we have good results in our application with this new functionality. For now closing the pull request.

@bbroeksema bbroeksema closed this Jun 30, 2015

@syntagmatic

This comment has been minimized.

Show comment
Hide comment
@syntagmatic

syntagmatic Jun 30, 2015

Owner

No objections! Great work @KPWhiver and thanks @bbroeksema for the merge and version bump. I quite like how http://syntagmatic.github.io/parallel-coordinates/examples/brushing.html looks with the colored shadows.

Owner

syntagmatic commented Jun 30, 2015

No objections! Great work @KPWhiver and thanks @bbroeksema for the merge and version bump. I quite like how http://syntagmatic.github.io/parallel-coordinates/examples/brushing.html looks with the colored shadows.

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