Added support for multi-line labels

[andr3nun3s]

This is related to issue #2402, more info there.

Don't merge yet as I need to add tests. Please review.

[mramato]

@Andre-Nunes is there a reason this is only showing up as one commit? Was @abwood's change already handling everything that was needed?

[andr3nun3s]

Yes, I found his commit and used it as a starting point, as it seemed to be working already I didn't make any changes, is that a problem?

It just needs testing.

[abwood]

Yes, I recall getting to the point of this working, but it was quick and
dirty and needed testing. Maybe ScaleByDistance issues with labels were
holding me back from a PR. Ultimately, around the time of this branch I
inherited some new responsibilities and it quickly fell off my radar.

Alex
On Mar 11, 2015 9:21 AM, "André Nunes" notifications@github.com wrote:

Yes, I found his commit and used it as a starting point, as it seemed to
be working already I didn't make any changes, is that a problem?

It just needs testing.

—
Reply to this email directly or view it on GitHub
#2575 (comment)
.

[mramato]

Because this is a common request, I would be okay with ScaleByDistance not working with it (and just writing up an issue for us to explore in the future). @Andre-Nunes can you add tests (One option would be to look inside the generated billboards and verify the offsets are what you expect). and let us know when it's ready to look at.

[andr3nun3s]

I'm sorry there hasn't been progress lately, I've been a bit busy with school.

I'm not sure how to calculate the expected offset by hand, I apologize but the math is a bit fuzzy on my side. I know pixelOffset is a Cartesian2 and the y offset depends on the number of newlines, the issue here is: how do I calculate that without replicating the code I'm trying to test. Do you see my point?

Thanks

[pjcozzi]

If we want this to make 1.8, please merge this coming week.

[pjcozzi]

Last call for 1.8. Up to you guys.

[andr3nun3s]

@mramato , I just need some tips on how to write the tests and I'll get this done today

[mramato]

Since the LabelCollection creates a BillboardCollection under the hood, it might be easiest to just just reach into the this._billboardCollection object and iterate over the individual billboards, making sure the y value in the pixel offset is properly computed. You'll want 2 tests, one that creates a multiline label without a LabelCollection.pixelOffset set and one that does it with an offset set. Don't worry about the x values of the offset, just the y values.

[andr3nun3s]

My silly question is: how do I compute the expected y without using the code I'm trying to test? Do you get what I mean? I'm probably missing something

[mramato]

While not normally good practice, I would make a simple 2 line string, something like I\nI, then visually inspect that it is correct in Sandcastle and then look at what the computed offsets are. Then just code those values into the test directly. Not perfect, but it will tell us when something breaks.

[andr3nun3s]

Thanks, I wasn't sure if that was a valid way to get the expected Cartesian values for the offset. I'll get to it!

[andr3nun3s]

I'm doing something wrong. Do I have to render the label during the test, how do I do that?

Right now the test looks something like this:

it('computes pixelOffset correctly for multiline labels', function() {
        var label = labels.add({
            position : Cartesian3.fromDegrees(0,0,0),
            text : 'HELLO\nWORLD'
        });

       // do I need to render the label?
       labels.update(context, frameState, []); 

        for (var i = 0; i < labels._billboardCollection._billboards.length; ++i) {
            var billboard = labels._billboardCollection._billboards[i];
            expect(billboard.pixelOffset.y).toEqual(-30);
        }
    });

[pjcozzi]

What's left for this?

[andr3nun3s]

I need help writing the tests, @mramato could you please take a look at my previous comment?

Thanks!

[emackey]

This came up on gis.stackexchange.com.

[mramato]

Closing this due to lack of movement. I might work on this as part of the code sprint this week.