Skip to content
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

Feature/multiple maps support #245

Merged
merged 27 commits into from
Nov 21, 2018
Merged

Feature/multiple maps support #245

merged 27 commits into from
Nov 21, 2018

Conversation

tomermoshe
Copy link
Contributor

@tomermoshe tomermoshe commented Nov 18, 2018

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all changes are documented in CHANGESLOG.md and README.md

@tomermoshe tomermoshe mentioned this pull request Nov 20, 2018
@tomermoshe
Copy link
Contributor Author

fix #243 #230 #212

* Fixed `ac-layer` onDestroy.
* Fixed `ac-map` onDestroy.
* Fixed `CircleEditorService` shape dragging.
* Fixed `ac-model-desc` docs [#243](https://github.com/TGFTech/angular-cesium/issues/243).
Copy link
Member

Choose a reason for hiding this comment

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

add plonter fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -44,7 +51,7 @@ export class AppComponent implements AfterViewInit {
ngAfterViewInit(): void {
// example for getting the viewer by Id outside of the ac-map hierarchy
const map = this.mapsManagerService.getMap('main-map');
const viewer = map.getCesiumViewer();
// this.mapsManagerService.bind2DMapsCameras(['main-map', 'sub-map']);
Copy link
Member

Choose a reason for hiding this comment

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

remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for demo purposes

<!--<html-layer></html-layer>-->
<!--<track-entity-layer></track-entity-layer>-->
<ac-map *ngFor="let map of maps; trackBy: mapsTrackBy" [sceneMode]="map.sceneMode" [mapId]="map.id" [containerId]="map.containerId">
<!-- <ac-map-layer-provider
Copy link
Member

Choose a reason for hiding this comment

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

why in comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we dont need it in the demo by default. The default map looks much better IMO

this.appSettingsService.showTracksLayer = true;

// setTimeout(() => this.showLayer = false, 5000);
// setTimeout(() => this.showMap = false, 5000);
Copy link
Member

Choose a reason for hiding this comment

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

remove, im not going to be your lint... check where there is unwanted comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For demo purposes

sceneMode = SceneMode.SCENE3D;
showLayer = true;
showMap = true;
maps = [
Copy link
Member

Choose a reason for hiding this comment

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

why there is an array here? its confusing
If we want an example for multi ac-map lets create a new component for it... after all multi map is used that often... and the demo has enough code without it...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -40,21 +40,22 @@
"postpublish": "npm run docs:push"
},
"peerDependencies": {
"@angular/common": "^4.2.6 || ^5.0.0 || ^6.0.0",
"@angular/core": "^4.2.6 || ^5.0.0 || ^6.0.0"
"@angular/common": "^4.2.6 || ^5.0.0 || ^6.0.0 || ^7.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

I guess we need to do the same for angular-parse...
angular 7 was tested with aot build on external project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

import { PointProps, PolylineProps } from './polyline-edit-options';
import { defaultLabelProps, LabelProps } from './label-props';
import { EllipseProps } from './ellipse-edit-options';

export class EditableCircle extends AcEntity {
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt it extend EditableEllipse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no

this._majorRadius = 0;
}
// } else if (this._center && this._majorRadiusPoint && this._minorRadius === undefined) {
// const newRadius = GeoUtilsService.distance(this.center.getPosition(), position);
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

}

endMovePolygon() {
this.lastDraggedToPosition = undefined;
this.positions.forEach(point => this.updatePointsLayer(true, point));
this.updatePolygonsLayer();
// this.positions.forEach(point => this.updatePointsLayer(true, point));
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed


activate(
options: {
onStart?: (acMap?: AcMapComponent) => any;
Copy link
Member

Choose a reason for hiding this comment

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

why the acMap is passed here?

Copy link
Contributor Author

@tomermoshe tomermoshe Nov 20, 2018

Choose a reason for hiding this comment

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

Because the service can be used without being provided and initialized by ac-map. In this case, this a way for the user to know in which map the zooming is taking place.

};
let borderElement: HTMLElement | undefined;

container.onmousedown = e => {
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt the event be configurable? like if i want zoom with right click.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can leave it like that for now. If we decide it needs modefing will do it in a new PR

};
let borderElement: HTMLElement | undefined;

container.onmousedown = e => {
Copy link
Member

Choose a reason for hiding this comment

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

do you unregister this events?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Instead the container is removed from the DOM and later gets picked by the garbage collector

}
this.disable(mapId);
const container = document.createElement('div');
mapContainer.style.position = 'relative';
Copy link
Member

Choose a reason for hiding this comment

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

isnt it a bit dangerous changing the uesrs map css??
why dont use fixed position?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The map container is created by us and not the user so I think its fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have something similar in ac-map


init(mapContainer: HTMLElement) {
init(mapContainer: HTMLElement, map: AcMapComponent) {
this.map = map;
this.ngZone.runOutsideAngular(() => {
const options = this.viewerConfiguration ? this.viewerConfiguration.viewerOptions : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

what about ViewerConfiguration how is it supports multi maps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added support through array

if (resultDistance > meterDistance) {
distanceFactorRangeMax = distanceFactorRangeMin + (distanceFactorRangeMax - distanceFactorRangeMin) / 2;
} else {
distanceFactorRangeMin = distanceFactorRangeMin + (distanceFactorRangeMax - distanceFactorRangeMin) / 2;
Copy link
Member

Choose a reason for hiding this comment

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

what about turf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It had the same issues as our original code

@eitanfr
Copy link
Member

eitanfr commented Nov 20, 2018

ac-layer onDestory?? doesnt need changes?

@eitanfr
Copy link
Member

eitanfr commented Nov 20, 2018

all data source and Z index code can be deleted??

@eitanfr
Copy link
Member

eitanfr commented Nov 20, 2018

add to README:

  1. maps manager - supports multi map better explanation about the api , and slave cameras
  2. all new services (ellipse,zoom...)

@eitanfr
Copy link
Member

eitanfr commented Nov 20, 2018

add resolved issues

@eitanfr eitanfr merged commit 85b7a51 into master Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants