Rewrite visualisation code

nlplab / brat

brat rapid annotation tool (brat) - for all your textual annotation needs

http://brat.nlplab.org

Other

1.82k stars 509 forks source link

Rewrite visualisation code #147

Closed amadanmath closed 13 years ago

amadanmath commented 13 years ago

The visualiser is too complex, and too slow, due to having grown organically.

Suggested rewrite:

Measure all new texts (creating elements off-DOM, then inserting all at once)
Calculate positions
Draw everything (off-DOM, then insert all at once) OR
Reposition everything (possibly by pulling from DOM, repositioning, reinserting)

This should minimise the number of browser layout operations, and also redundant measuring.

spyysalo commented 13 years ago

Agree, but suggest to delay until after the next fully stable version is released with the features currently planned.

Also, I think it should be possible to cache some of the measurements regarding e.g. font metrics. As the same strings often recur with the same style, perhaps we can avoid drawing every "the" and "Protein" twice. (Or is this perhaps what you're suggesting also?)

amadanmath commented 13 years ago

Indeed, this is what I am suggesting: for each font style (i.e. separately the text, the annotations and the arc names), make a hash of all unique strings that appear. Currently, not only is "the" measured repeatedly, but even more than once per instance (once without box, then once with box, then as a chunk... far from optimal)

Also, very much agreed on the delay: this is scary, and likely to take up much time, and introduce a host of new bugs, and is also the reason why i did not assign a milestone :)

spyysalo commented 13 years ago

Assigned to new "visualizer rewrite" milestone, tentatively marked due for ACL.

amadanmath commented 13 years ago

Scratch that, I was completely wrong. Testing I did today show that the delay is indeed caused by the measurement itself, and that incremental updates to the DOM help and not hurt. On a particular document, I was just drawing all the text, then measuring it (or not). The median values:

Chrome, no measurement: 11ms
Chrome, with measurement, DOM or off-DOM: 17ms
Firefox, no measurement: 22ms
Firefox, with measurement, incremental building: 25ms
Firefox, with measurement, off-DOM building: 542ms (!)

What a difference an instruction makes! and no, I have no idea why. :( But I'll try to take it into consideration.

spyysalo commented 13 years ago

Great! I take it we should then be able to support firefox without too great changes to the visualization code. Let's try to get a version that doesn't choke on firefox done for ACL and do a new stav release based on it.

spyysalo commented 13 years ago

With the integration of newvis and temporary retirement of our long-suffering JS developer, I think we can call this one done!