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.

Sign up for GitHub

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

Jump to bottom

Vectors without hard ol3 pr dependency #42

Merged
merged 25 commits into from
Sep 12, 2014
Merged

Vectors without hard ol3 pr dependency #42

merged 25 commits into from
Sep 12, 2014

Conversation

gberaudo
Copy link
Member

This PR is almost the same as #33.
It works with ol3 master, though it would benefit from the merge of the ol3 PRs.
I will merge it today, so that we can move on.

petrsloup
petrsloup reviewed Sep 12, 2014
View reviewed changes
src/rastersynchronizer.js
@@ -103,7 +103,8 @@ olcs.RasterSynchronizer.prototype.synchronize = function() {
}, this);

this.olLayers_.forEach(function(el, i, arr) {
synchronizeLayer(el);
if (el.getSource() instanceof ol.source.TileImage)
Copy link
Member

Choose a reason for hiding this comment

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

This breaks LayerGroup support. The getSource method is not defined in that case!

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I will check using your patch #34.

On Fri, Sep 12, 2014 at 05:57:34AM -0700, Petr Sloup wrote:

@@ -103,7 +103,8 @@ olcs.RasterSynchronizer.prototype.synchronize = function() {
}, this);

this.olLayers_.forEach(function(el, i, arr) {

  • synchronizeLayer(el);
  • if (el.getSource() instanceof ol.source.TileImage)

This breaks LayerGroup support. The getSource method is not defined in that case!

Reply to this email directly or view it on GitHub:
https://github.com/boundlessgeo/ol3-cesium/pull/42/files#r17476808

Guillaume Beraudo
Camptocamp Engineer

Copy link
Member Author

Choose a reason for hiding this comment

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

If you can rebase your PR on top of #41 I will merge it today.
Otherwise I will bundle it together with my commits today.

Copy link
Member

Choose a reason for hiding this comment

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

#34 rebased on master, so it can be merged easily.
If there's any problem with that, feel free to make it part of your PR.

@petrsloup
Copy link
Member

I have some major comments about this PR:

  • There are some build configuration changes as part of this PR -- would it be possible to make them a separate PR as they are actually unrelated to the Vector layer synchronization?
  • There are many "global" function (not actually global, because the whole content of the file is wrapped inside "anonymous" function) -- this should be placed inside the namespace as well (without the @api annotation, so the build size is not increased).
  • The olcs.core is getting very large -- I would suggest to split it into several files (olcs.core.vector, olcs.core.style, ... ?).

I also added some minor comments and questions directly to the files.

@gberaudo
Copy link
Member Author

I updated the PR with:

  • fix for raster synchronizer and layer groups;
  • fix for vector synchronizer and layer groups;
  • fix for constant-like initialization of core.

I also answered the questions about:

  • why the PR includes config for enabling compilation checks;
  • the many advantages of the anonymous function technique.

The core namespace looks pretty clean for me. Conversely I feel the
core.js is becoming fat and would benefit from a split, but this will
have to wait for after the merging of this PR.

@gberaudo
Externs for Code review + compiler checks activation.
0e1ed00
@gberaudo
Externs for Text aligment, baseline and offset + style fallback fix.
47cda56
@gberaudo
Externs for Prevent minification of current vector structs passed to
4c5ddc9
@gberaudo
Geometry converter core function and example.
43ce5e8 
Labels and Points are not handled by a geometry in Cesium.
Showing console error for now.
@gberaudo
Vector synchronizer.
1f439fc 
Implemented on the model of RasterSynchronizer.
@gberaudo
Code review + compiler checks activation.
37bf961
@gberaudo
Icon style test.
d319daf
@gberaudo
Text aligment, baseline and offset + style fallback fix.
0376f73
@gberaudo
Add KML/... drag&drop interactions on ol3 map.
23d9121
@gberaudo
Reworked global variable for line width
3a5f515
@gberaudo
Listen for event the Ol3 way.
b28435a
@gberaudo
Use Array.forEarch.
9575d19
@gberaudo
Wrap core functions in anonymous function.
c909ca7 
Prevent polution of the window object.
@gberaudo
Write combined exports to dist/exports.js.
6f92bbf 
Necessary for unminification with generated source map.
@gberaudo
Generate source map and modify some compile flags.
591ba18
@gberaudo
Do not use strict in core.js has it breaks compiled code.
551e990 
Breaks in google code detecting the browser user agent.
@gberaudo
Wrap our Cesium primitives in a collection.
06abfdd 
Allows removing all of our primitives and keeping foreign ones.
@gberaudo
Prevent minification of current vector structs passed to Cesium.
26dc4bc 
Though it 'works', this solution is extremely fragile due to limitations
in the way Closure handle structs. Notably, we do not have:
- check for typos/invalid properties;
- enforcement of required properties;

Using proper types declared with @struct is cumbersome and would still
be slippery:
+ detect typos if used correctly;
- less readable;
- even more externs.
@gberaudo
Depend on Cesium 1.1.
b3df98c
@gberaudo
Distribute examples
9befb17
@gberaudo
Support multipoint features.
f0c0e97
@gberaudo
Fix polygons with holes conversion.
1298042
@gberaudo
Depend on ol3 master.
ff4915d
@gberaudo
Throw when glAliasedLineWidthRange is not initialized.
7e01c3c 
This library state is initialized in vectorsynchronizer.
If not using the synchronizer, the user must inialize the library herself.
How to initialize the core must be added to our future documentation.
@gberaudo
Fix vector synchronizer with layer groups.
ce250f8
gberaudo added a commit that referenced this pull request Sep 12, 2014
@gberaudo
Merge pull request #42 from camptocamp/vectors_without_hard_ol3_pr_de…
ac5483a 
…pendency

Vectors without hard ol3 pr dependency.
@gberaudo gberaudo merged commit ac5483a into openlayers:master Sep 12, 2014
This was referenced Sep 12, 2014
Closed
Merged
@elemoine
Copy link
Member

I used the vector synchronization example for a demo at FOSS4G yesterday, and I experienced a problem where the vector features disappeared at some point, and then reappeared.

@gberaudo
Copy link
Member Author

Hi Eric,

It is done on purpose.
With a timer, one ol3 layer is removed then added back.

This was referenced Sep 16, 2014
Closed
Closed
@gberaudo gberaudo deleted the vectors_without_hard_ol3_pr_dependency branch September 24, 2014 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gberaudo @petrsloup @elemoine @tschaub @klokan