Used Label.java to implement Placemark label drawing

Jeffrey-P-McAteer commented 4 years ago

Description of the Change

Implemented label drawing on Placemark object (todo comment). This stores a Label object within Placemark which may not be desired.

Why Should This Be In Core?

This is a documented feature which has not been implemented; it is desirable to have documented APIs be implemented. The workaround for this is having all users keep track of a Placemark and Label themselves, and remembering to move both items when they change position, and keep other properties in sync.

Benefits

Placemark now draws the label text set via Placemark.setLabel.

Potential Drawbacks

This uses a Label object and during each render pass it assigns Placemark.activeAttributes.getLabelAttributes to Label.setAttributes. I am not sure what performance penalty there is and there may be a better way to track when Placemark.activeAttributes gets a label attributes object assigned. From testing with 5 icons there was no noticeable performance effect.

Applicable Issues

Possibly NASAWorldWind issue 27, but that sounds like it is more abstract and applies to more shapes than just Placemark.

Attribution

This work was done by DeVil-Tech to support our Locorum project.

ComBatVision commented 4 years ago

@Jeffrey-P-McAteer From my opinion this enhancement adds new unexpected behavior.

Experimental MilStd2525Placemark pass symbol code as placemark name in its constructor.

    public MilStd2525Placemark(Position position, String symbolCode, SparseArray<String> modifiers, SparseArray<String> attributes) {
        super(position, null /* attribute bundle */, symbolCode /* name */);

Name attribute by default is used to determine Placemark DisplayName just for some identification purposes like in Layer or any visible item. Your enhancement adds setLable into constructor and make name always equals label text.

    public Placemark(Position position, PlacemarkAttributes attributes, String name) {
        this.setPosition(position);
        this.setAltitudeMode(WorldWind.ABSOLUTE);
        this.setDisplayName(name == null || name.isEmpty() ? "Placemark" : name);
        this.setLabel(name);

Method setLabel creates default TextAttributes inside Label constructor.

    public Label(Position position, String text) {
        if (position == null) {
            throw new IllegalArgumentException(
                Logger.logMessage(Logger.ERROR, "Label", "constructor", "missingPosition"));
        }

        this.position.set(position);
        this.text = text;
        this.attributes = new TextAttributes();

As the result SymbolID is always displayed as a Placemark Label because lable text equals name and LableTextAttributes is not null by default.

I think Label text and DisplayName should not be always equal and Label text should be Controlled explicitly. What do you think?

And another suspicious thing. Label and Placemark has separate position objects inside (see Label constructor above) and always copy lat, lon and alt from incoming position object using set method. Looking at current code Label should not move after the Placemark position, but for some unexpected reason it moves. This is a correct behavior for Lablel to follow Placemark, but, please, clarify how Label follow Placemark position?

ComBatVision commented 4 years ago

I found code which moves position of Label in separate commit. Question about equality of displayName and label text is still open.

ComBatVision commented 4 years ago

@Jeffrey-P-McAteer I have reverted develop head. Could you, please, cherry-pick all commits related to Placemark Label to some new branch e.g. "feature/placemark-label" in you repository fork starting with some commit already existed in our develop and create new PR with description copied from this PR, so I can review and merge only this feature?

WorldWindEarth / WorldWindAndroid