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

add JSONObject Attribute to batchTable #65

Merged
merged 1 commit into from
Feb 22, 2016

Conversation

jbo023
Copy link
Contributor

@jbo023 jbo023 commented Jan 28, 2016

We are working with datasets which often have inhomogeneous or even nested object attribute datasets.

Because of this we would prefer if the specification doesn't set strict presets on the allowed datatype for batchTable attributes. With inhomogeneous data this leads to a Batchtable where many fields are "null".

In the pull request I just added JSONObject to the allowed datatypes. This will allow us to add complex data as an batchTable attribute. We have done some testing with this and it actually already works with cesium, as long as the batchtable is a valid JSON.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 28, 2016

Thanks @jbo023! I am actually just now starting to flush out the 3D Tiles properties access in the Cesium API so this is good timing. Give me a few days to get to this.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 16, 2016

@jbo023 I need to look more carefully at this Cesium implementation, but this is OK with me if it doesn't have any big implication. From memory, I think it is fine.

For the declarative styling (#2) we would just implement the member operator (.) to allow access to sub-properties.

@pjcozzi pjcozzi mentioned this pull request Feb 19, 2016
85 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 22, 2016

@jbo023 sorry for the slow response here, but I reviewed the spec and Cesium code and talked this over with @tfili who did some of the implementation work, and it should be fine to store Object or even Array here.

@lilleyse could you update the unit tests in Cesium with an example where properties are objects and arrays before we update the spec here?

@mramato
Copy link

mramato commented Feb 22, 2016

I'm sure there's a good answer to this, but what is the type information used for outside of parsing? From a parsing implementation standpoint, arrays of strings, numbers, and booleans are also valid JSON so technically it's all the same type (JSON). Is there a need for the client to know the type after parsing is done, thus requiring it to be specifically specified? Is it for validation purposes?

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 22, 2016

The original intention might have been to avoid some clones on the client, but we already using it.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 22, 2016

Cesium tests are in CesiumGS/cesium#3616.

Thanks again, @jbo023. I'm going to merge this and open another pull request to update this a bit more and also update the i3dm tile format.

pjcozzi added a commit that referenced this pull request Feb 22, 2016
add JSONObject Attribute to batchTable
@pjcozzi pjcozzi merged commit 0614a16 into CesiumGS:master Feb 22, 2016
@arneschilling
Copy link

@pjcozzi : thanks a lot
@mramato : the in built JSON parser of the browser shouldnt complain about it. We just just want to avoid the allowed datatypes being whitelisted in Cesium. We need the client implementation to know the type in order to display it correctly, but that's not an issue.

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.

None yet

4 participants