BRP improve json schema support #19786
Conversation
Co-authored-by: Gino Valente <[email protected]>
Co-authored-by: Gino Valente <[email protected]>
# Conflicts: # crates/bevy_remote/Cargo.toml # crates/bevy_remote/src/builtin_methods.rs # crates/bevy_remote/src/schemas/json_schema.rs # crates/bevy_remote/src/schemas/mod.rs
janhohenheim
added
the
S-Needs-Review
|
@mweatherley @splo @MrGVSV Could be interested in this :) It still requires some cleanup for sure before merging, but I wanted to make sure if that is something that others find useful enough to justify changes. |
|
I also was wondering about adding dev dependency: https://github.com/GREsau/schemars |
Sounds great, I will check it out tomorrow/today. Edit@ Just did it, it works! Great :D |
alice-i-cecile
added
the
X-Controversial
|
When running the example server, it panics with the following error: It seems it fails calling this at self.register_force_as_array::<bevy_math::I8Vec2>(); |
with pattern and property_names
|
@splo I've updated the registration process, it should be fine now. @ChristopherBiscardi I still want to adress your concern about loosing field informations. What do you think about introducing additional field {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"shortPath": "DAffine3",
"typePath": "glam::DAffine3",
"modulePath": "glam",
"crateName": "glam",
"reflectTypeData": [
"Deserialize",
"Serialize",
"Default"
],
"kind": "Struct",
"rustFieldsInfo": {
"translation": {
"$ref": "#/$defs/glam%3A%3ADVec3",
"title": "translation",
"typePath": "glam::DVec3",
"kind": "Struct"
},
"matrix3": {
"$ref": "#/$defs/glam%3A%3ADMat3",
"title": "matrix3",
"typePath": "glam::DMat3",
"kind": "Struct"
}
},
"type": "array",
"prefixItems": [
{
"title": "matrix3.x_axis.x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.x_axis.y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.x_axis.z",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.y_axis.x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.y_axis.y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.y_axis.z",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.z_axis.x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.z_axis.y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "matrix3.z_axis.z",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "translation.x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "translation.y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "translation.z",
"typePath": "f64",
"kind": "Value",
"type": "number"
}
],
"minItems": 12,
"maxItems": 12,
"$defs": {
"glam::DMat3": {
"shortPath": "DMat3",
"typePath": "glam::DMat3",
"modulePath": "glam",
"crateName": "glam",
"reflectTypeData": [
"Deserialize",
"Serialize",
"Default"
],
"kind": "Struct",
"rustFieldsInfo": {
"x_axis": {
"$ref": "#/$defs/glam%3A%3ADVec3",
"title": "x_axis",
"typePath": "glam::DVec3",
"kind": "Struct"
},
"y_axis": {
"$ref": "#/$defs/glam%3A%3ADVec3",
"title": "y_axis",
"typePath": "glam::DVec3",
"kind": "Struct"
},
"z_axis": {
"$ref": "#/$defs/glam%3A%3ADVec3",
"title": "z_axis",
"typePath": "glam::DVec3",
"kind": "Struct"
}
},
"type": "array",
"prefixItems": [
{
"title": "x_axis.x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "x_axis.y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "x_axis.z",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "y_axis.x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "y_axis.y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "y_axis.z",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "z_axis.x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "z_axis.y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "z_axis.z",
"typePath": "f64",
"kind": "Value",
"type": "number"
}
],
"minItems": 9,
"maxItems": 9
},
"glam::DVec3": {
"shortPath": "DVec3",
"typePath": "glam::DVec3",
"modulePath": "glam",
"crateName": "glam",
"reflectTypeData": [
"Deserialize",
"Serialize",
"Default"
],
"kind": "Struct",
"rustFieldsInfo": {
"x": {
"title": "x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
"y": {
"title": "y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
"z": {
"title": "z",
"typePath": "f64",
"kind": "Value",
"type": "number"
}
},
"type": "array",
"prefixItems": [
{
"title": "x",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "y",
"typePath": "f64",
"kind": "Value",
"type": "number"
},
{
"title": "z",
"typePath": "f64",
"kind": "Value",
"type": "number"
}
],
"minItems": 3,
"maxItems": 3
}
}
}What do you think? Same question to the @splo |
|
Before this PR, the registry query lied about some of its types. For instance, in my VS Code extension, I have code like this: if (typePath === 'glam::Vec3`) {
// Ignore that this pretends to be an object with x, y, z properties.
// Instead treat it as an array with 3 elements.
}I reported this issue earlier in this PR and I thought we concluded that we'd tackle it later. Indeed, the issue is not only with The current state of this PR tries to manually fix the bevy_math types by hard-coding them while I hoped the fix would be more general and cover all such cases. I understand this is a tricky issue. So first question: is it ok to have a temporary and incomplete workaround in this PR? If so and we naively fix Second question: is it that important? What is the use case for reporting the internal Rust type information in the registry query? I feel that is an actual controversial change. I think most BRP users only need to know the actual types they are supposed to work with. Is it about code generation? Like read type info from BRP and generate Rust code that should be compiled with Bevy types? If we decide to somehow report the internal Rust type information, then yes, I guess we could include that in a custom field. In the current format, is it possible to map the internal Rust field to its associated JSON Schema field? I mean for Anyway, how about concentrating all custom fields (such as "glam::Vec3": {
"type": "array",
"maxItems": 3,
"minItems": 3,
"prefixItems": [
{
"title": "x",
"type": "number",
"bevyType": {
"kind": "Value",
"typePath": "f32"
}
},
{
"title": "y",
"type": "number",
"bevyType": {
"kind": "Value",
"typePath": "f32"
}
},
{
"title": "z",
"type": "number",
"bevyType": {
"kind": "Value",
"typePath": "f32"
}
}
],
"bevyType": {
"crateName": "glam",
"kind": "Struct",
"modulePath": "glam",
"reflectTypeData": ["Default", "Serialize", "Deserialize"],
"shortPath": "Vec3",
"typePath": "glam::Vec3",
"fields": {
"x": {
"kind": "Value",
"typePath": "f32"
},
"y": {
"kind": "Value",
"typePath": "f32"
},
"z": {
"kind": "Value",
"typePath": "f32"
}
}
}
}It would isolate non-standard fields and make it easier to parse the JSON Schema. It would also allow us to add more custom fields in the future without cluttering the main schema structure. |
@splo I think that yes, given that Bevy Registry does not give us information that we could use to detect and handle those cases. One other thing that this does not cover is serde attributes, but support for that must wait for #19557 .
I think that not loosing information that we can provide is useful, but maybe it should all be in separate field like you described later?
For this format I was thinking about what format for title would be useful to cover cases like the one here: https://github.com/rust-adventure/skein/blob/main/extension/object_to_form.py I really like the idea of putting bevy specific fields into one field, but I don't know if that should be part of this PR tbh. |
|
@mweatherley @MrGVSV How do you feel about the current state of the PR? |
I need more context information to understand exactly why the Rust internal structure is needed in this Python code. But fine, if there's demand for this feature, we can add it.
It could be in another PR of course. I just fear a PR with only cosmetic schema-breaking changes would be de-prioritized to hell. Like "too late, we can't change anything anymore, the API is frozen forever". I've seen that in a professional context so many times. |
Yeah, I get that :< I agree with you, but I am already a bit worried if that wouldn't make this PR too big. It already did have a long discussion and I would love to make it to the finish line before 0.17. Right now I would love to have some feedback from the team to know if that can make into it and/or what would need to be changed or fixed. |
| } | ||
|
|
||
| /// Helper trait | ||
| pub(crate) trait RegisterReflectJsonSchemas { |
There was a problem hiding this comment.
Does this need to be a trait with blanket implementations as opposed to simple functions?
There was a problem hiding this comment.
That way it is implemented for both App and TypeRegistry, making it easier to use.
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| ReferenceLocation::Definitions => write!(f, "#/$defs/"), | ||
| ReferenceLocation::Components => write!(f, "#/components/"), |
There was a problem hiding this comment.
The fact that components and $defs are hard-coded here is pragmatic and fine. But don't you think in an ideal situation, these should be somehow passed as parameters by the calling code (which is json_schema or open_rpc)?
There was a problem hiding this comment.
Agree, but I that could be a separate improvement.
There was a problem hiding this comment.
Maybe add a comment for this? I don't know Bevy maintainers stance on TODO comments but there could be an indication on how it should be improved in the future.
| } | ||
| } | ||
|
|
||
| /// JSON Schema type for Bevy Registry Types | ||
| /// It tries to follow this standard: <https://json-schema.org/specification> | ||
| /// | ||
| /// To take the full advantage from info provided by Bevy registry it provides extra fields | ||
| #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Default)] | ||
| #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Default, Reflect)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct JsonSchemaBevyType { |
There was a problem hiding this comment.
Just a thought: if we used a third-party library schema type (jsonschema), we would have a standard to rely on. We could include custom Bevy types with a struct that flattens the official fields:
pub struct BevyJsonSchema {
#[serde(flatten)]
pub schema: Schema, // Third-party schema.
// Bevy extensions:
pub short_path: Cow<'static, str>,
pub type_path: Cow<'static, str>,
// etc.
}There was a problem hiding this comment.
Overall this file is very well documented, congratulations! It is very long though. Once we take the time to read it, it's not too complicated.
Do you think a third-party library would help here to lower the line count and ease future maintenance?
|
@splo thanks for the feedback! About the last two pieces- I think maybe it would make sense to move the schema stuff to separate crate inside bevy repo since it depends so much on bevy reflection. |
|
@MrGVSV I've tried to update first message in PR as well, do you have any opinions on the PR? |
I understand much of the code rely on
|
Objective
Currently implemented JSON Schema export is often producing invalid schemas for types resulting in that json that would be a valid value for schema generated for type cannot be deserialized to that type by Bevy deserializer.
Solution
This PR tries to reimagine how the schema is constructed. It follows Draft 2020 of JSON schema, as definied here: https://json-schema.org/draft/2020-12/schema
It is possible to use schemas generated by new system as validation data for assets in various editors. Once there would be a support for getting information about supported extensions with reflection system I could write example. Related task: #19399
Main additions and change to previous behaviour would be:
f32generates{"type": "number", "shortPath": "f32", "typePath": "f32"}instead of{"$ref": "#/$defs/f32"}CustomInternalSchemaDataenables providing custom schemas for reflected typesVec3that serialize as 3-element arrays despite being structs with 3 fieldsExternalSchemaSourcetrait allows referencing external schema sourcesReflectDefaultimplementation automatically include adefaultfield in their schema#[reflect(@min..=max)]) are properly handledOption<T>is now treated as a required field with possible values ofTornull. This approach ensures compatibility with JSON Schema validation while maintaining type safety. Information about whether field is required should be provided by bevy reflection system, until that is the case we cannot guarantee that data with missing optional field is would be parsed correctly. Related task: Access serde attributes with reflection #19557rustFieldsInfo- It is provided when type is serialized as array, but the type is not an array. It is done to provide additional information about the fields in the schema.Example schema:
{ "$schema": "https://json-schema.org/draft/2020-12/schema", "shortPath": "DAffine3", "typePath": "glam::DAffine3", "modulePath": "glam", "crateName": "glam", "reflectTypeData": [ "Deserialize", "Serialize", "Default" ], "kind": "Struct", "rustFieldsInfo": { "translation": { "$ref": "#/$defs/glam%3A%3ADVec3", "title": "translation", "typePath": "glam::DVec3", "kind": "Struct" }, "matrix3": { "$ref": "#/$defs/glam%3A%3ADMat3", "title": "matrix3", "typePath": "glam::DMat3", "kind": "Struct" } }, "type": "array", "prefixItems": [ { "title": "matrix3.x_axis.x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.x_axis.y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.x_axis.z", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.y_axis.x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.y_axis.y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.y_axis.z", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.z_axis.x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.z_axis.y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "matrix3.z_axis.z", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "translation.x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "translation.y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "translation.z", "typePath": "f64", "kind": "Value", "type": "number" } ], "minItems": 12, "maxItems": 12, "$defs": { "glam::DMat3": { "shortPath": "DMat3", "typePath": "glam::DMat3", "modulePath": "glam", "crateName": "glam", "reflectTypeData": [ "Deserialize", "Serialize", "Default" ], "kind": "Struct", "rustFieldsInfo": { "x_axis": { "$ref": "#/$defs/glam%3A%3ADVec3", "title": "x_axis", "typePath": "glam::DVec3", "kind": "Struct" }, "y_axis": { "$ref": "#/$defs/glam%3A%3ADVec3", "title": "y_axis", "typePath": "glam::DVec3", "kind": "Struct" }, "z_axis": { "$ref": "#/$defs/glam%3A%3ADVec3", "title": "z_axis", "typePath": "glam::DVec3", "kind": "Struct" } }, "type": "array", "prefixItems": [ { "title": "x_axis.x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "x_axis.y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "x_axis.z", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "y_axis.x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "y_axis.y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "y_axis.z", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "z_axis.x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "z_axis.y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "z_axis.z", "typePath": "f64", "kind": "Value", "type": "number" } ], "minItems": 9, "maxItems": 9 }, "glam::DVec3": { "shortPath": "DVec3", "typePath": "glam::DVec3", "modulePath": "glam", "crateName": "glam", "reflectTypeData": [ "Deserialize", "Serialize", "Default" ], "kind": "Struct", "rustFieldsInfo": { "x": { "title": "x", "typePath": "f64", "kind": "Value", "type": "number" }, "y": { "title": "y", "typePath": "f64", "kind": "Value", "type": "number" }, "z": { "title": "z", "typePath": "f64", "kind": "Value", "type": "number" } }, "type": "array", "prefixItems": [ { "title": "x", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "y", "typePath": "f64", "kind": "Value", "type": "number" }, { "title": "z", "typePath": "f64", "kind": "Value", "type": "number" } ], "minItems": 3, "maxItems": 3 } } }Testing