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

Editor data model #62

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

djeedai
Copy link

@djeedai djeedai commented Jul 19, 2022

RENDERED

This RFC is for the (upcoming) Bevy Editor, and has little to no impact on the Bevy runtime itself (existing code).

This RFC describes the data model of the Editor, that is the representation of the Game data and the API to manipulate it that the Editor uses. The API heavily makes use of the Reflect API of Bevy to manipulate type erased objects.

The data model aims at having the following properties:

  • The data mutations are diff-based instead of allowing direct object mutations. This allows all systems of the Editor to access a unique source of truth, with the added benefits of allowing to build unified undo/redo and copy/paste systems.
  • The data model is an abstraction of the game data, which does not require a direct dependency on the Game code (no Cargo reference), and therefore avoids the need to rebuild or even restart the Editor process each time the Game code changes.
  • The data representation can be serialized to disk and deserialized back (round-trip) without loss of data.

The details and mechanisms by which:

  • this data model is effectively serialized (via reflection)
  • the Game and Editor processes actually communicate (inter-process communication; IPC)
  • the editing data is baked into its final shipping-ready form (asset processing)

are all outside the scope of this RFC.

@Nilirad
Copy link

Nilirad commented Jul 20, 2022

Can you please put a RENDERED link at the top of your message, pointing to the .md file? 🙂

@djeedai
Copy link
Author

djeedai commented Jul 20, 2022

Can you please put a RENDERED link at the top of your message, pointing to the .md file? 🙂

Sorry I saw this was common practice but then I forgot. 😅

@Nilirad
Copy link

Nilirad commented Jul 20, 2022

The file name has still the 301 “status code”, I think you should rename it to 62. 🙂

@djeedai
Copy link
Author

djeedai commented Jul 20, 2022

The file name has still the 301 “status code”, I think you should rename it to 62. 🙂

I picked a large number on purpose because it's for the Editor. Not sure what's the usual convention, and/or if we want a different one for the Editor (which will be the first official Bevy executable) or if we keep those mixed with Bevy (the library crates)?

@mockersf
Copy link
Member

the number is for tracking, and it's the PR number

@djeedai
Copy link
Author

djeedai commented Jul 20, 2022

the number is for tracking, and it's the PR number

Ok so should I rename the file then?

@alice-i-cecile
Copy link
Member

Yes please :)

@djeedai
Copy link
Author

djeedai commented Jul 20, 2022

Yes please :)

Done.

Copy link

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Just reviewing the User-facing explanation for now, since the RFC is quite complex and I've spent much energy to review it so far. It looks decent overall, but many points should be made more clear. I'll make here some general observations about what I've seen.

I strongly recommend you to use semantic line breaks, as they help a lot the reviewing process. Single-line paragraphs are not very ergonomic, and make it difficult to pinpoint an issue. Start by giving the entire line to each sentence, then if you feel like it, break sentences into their logical pieces. See sembr.org for more about semantic line breaks.

I think the “Motivation” section should enumerate the Editor properties that we want, and how the data model helps us to achieve that.

In general, the User-facing explanation seems to ignore/hide that Game dynamic types are sent to the Editor via the Reflection API. Instead it often just refers to bare types.

It should also pinpoint, as a higher level overview, that there are other types of data model (which won't need discussion here): the serialized, persistent storage version of the Editor data model, and the Runtime data model. Those are mentioned and may cause confusion sometimes.

The User-facing explanation seems a big monolith piece. I think you should use sections to break it into smaller pieces. It will both help readability since topics are separated, and it will help you to see better what points are too short and need to be furtherly developed.


## Motivation

We need a data model for the Bevy Editor. We want to make it decoupled from the runtime data model that the Game uses, which is based on compiled Rust types (`World`, _etc._) with a defined native layout from a fixed Rust compiler version (as most types don't use `#[repr(C)]`), so we can handle data migration across Rust compiler versions and therefore across both Editor and Game versions. We also want to make it _transportable_, that is serializable with the intent to transport it over any IPC mechanism, so that the Editor process can dialog with the Game process to enable real-time Game editing ("Play Mode").
Copy link

Choose a reason for hiding this comment

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

I had quite a hard time understanding this paragraph, as some stances are not justified, and the language is unclear in some points. I also think this paragraph should be separated in at least two paragraphs to separate concepts.

After a second reading, I have much less difficulty to partially understand this, so I think the culprit here is usage of terms that are not yet been introduced.

We need a data model for the Bevy Editor.

What are the motivation behind this? Also, what is a data model? It should be defined before, not after it is used.

compiled Rust types

Technically, machine code is untyped, so I don't know how much sense it makes.

native layout

What does native layout mean? We should probably use a clearer expression.

handle data migration across Rust compiler versions

Why do we really need to migrate data even when changing compiler version? This needs to be explained.

We also want to make it transportable

What is “it”? The data in the editor data model format?

to enable real-time Game editing ("Play Mode").

I think this clause can be omitted. Readers just need to know that we're required to transport data from the Editor back to the Game through IPC.

Copy link
Author

Choose a reason for hiding this comment

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

What does native layout mean? We should probably use a clearer expression.

Machine layout? Binary layout?

Readers just need to know that we're required to transport data from the Editor back to the Game through IPC.

Readers here are RFC reviewers right? If so, I'd argue they need to understand why we use IPC.

Copy link

Choose a reason for hiding this comment

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

What does native layout mean? We should probably use a clearer expression.

Machine layout? Binary layout?

The problem is that it's difficult to understand what you're referring about here. The thing is that all data structures are just an abstraction introduced by high level programming language, and this abstraction is lost just right when the code is translated into assembly, where the fields of the data structure defined in the high level programming language are accessed with a base pointer + offset method. (Probably not 100% correct since i don't have much experience here, but the gist is that)

Personally, I would just avoid talking about machine code representations of code, when possible. The reason that we want the Editor to have a different data model from the Runtime's is because they, by our requirement, do not depend on each other. The machine-code incompatibility between them is just a consequence of that.

Readers just need to know that we're required to transport data from the Editor back to the Game through IPC.

Readers here are RFC reviewers right? If so, I'd argue they need to understand why we use IPC.

Who reviews the RFC is just required to know the current state of the project, the already merged RFCs and what has been defined in the RFC up to that point, so they may not know about concepts developed in the bevyengine/bevy#5043 discussion.

However, in that case, I didn't ask to add information, but to remove some, because it seems not relevant to the RFC. We need a data model that supports IPC between the Editor and the Game (because we want them to be independent from each other), but it seems inappropriate to talk about the “Play Mode” here, where it's not mentioned anywhere else apart from the Unity prior art section.

Copy link
Author

Choose a reason for hiding this comment

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

The thing is that all data structures are just an abstraction introduced by high level programming language, and this abstraction is lost just right when the code is translated into assembly, where the fields of the data structure defined in the high level programming language are accessed with a base pointer + offset method.

The native layout is the offsets. This is the part which is not lost. I don't have a better term for it, that's the one I commonly use and hear used to talk about this.

Personally, I would just avoid talking about machine code representations of code, when possible. The reason that we want the Editor to have a different data model from the Runtime's is because they, by our requirement, do not depend on each other. The machine-code incompatibility between them is just a consequence of that.

Fair enough. The native layout will probably be relevant for another RFC but not this one, will remove.

but it seems inappropriate to talk about the “Play Mode” here

OK sorry I thought the issue was with the mention of IPC, not about Play Mode. I can remove that one.

Copy link
Author

Choose a reason for hiding this comment

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

Removed both native layout and Play Mode references.

rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved

The _data model_ of the Editor refers to the way the Editor manipulates editing data, their representation in memory while the Editor executable is running, and the API to do those manipulations. The Editor at its core defines a centralized data model, and all systems use the same API to access that data model, making it the unique source of truth while the Editor process is running. This unicity prevents desynchronization between various subsystems, since they share a same view of the editing data. It also enables global support for common operations such as copy/paste and undo/redo without requiring each subsystem to implement their own variant.

A Bevy application (the Game) uses Rust types defined in one of the `bevy_*` crates, any other third-party plugin library, or the Game itself (_custom types_). The data is represented in memory at runtime by _instances_ (allocated objects) of those Rust types. This representation is optimal in terms of read and write access, but is _static_, that is the type layout is defined during compiling and cannot be changed afterward.
Copy link

Choose a reason for hiding this comment

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

A Bevy application (the Game)

We can simply refer to it as the Game, since it's already defined.

A Bevy application (the Game) uses Rust types defined in one of the bevy_* crates, any other third-party plugin library, or the Game itself (custom types).

You missed the types in the std library and types in non-plugin library. Also I'm questioning if this sentence is relevant to the RFC.

Copy link
Author

Choose a reason for hiding this comment

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

You missed the types in the std library and types in non-plugin library.

Again, I've never said or written that all Rust types are concerned by the IPC mechanism. I very much doubt the Editor cares about Option<T> as a type; we probably want to limit to components, resources, events, and a few others relevant for editing.

I'm not sure what you mean by "non-plugin library" though.

Also I'm questioning if this sentence is relevant to the RFC.

This shows the contrast between the Game data model and the Editor one, and why we cannot just have a single data model for both.

Copy link

Choose a reason for hiding this comment

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

Again, I've never said or written that all Rust types are concerned by the IPC mechanism.

Oops, I thought you tried to be exhaustive when enumerating the types that the Game was aware about. It looks like you're just listing what Game types need to be shared (through reflection) with the Editor.

I'm not sure what you mean by "non-plugin library" though.

I was meaning a dependency of the Game that is not a Bevy plugin, e.g. the rand crate. But again, probably not something we want the Editor to care about.

This shows the contrast between the Game data model and the Editor one, and why we cannot just have a single data model for both.

I think there is a more explicit and clear way to expose that. It probably should be mentioned in the beginning of the paragraph, such as “The Editor and Game data models are of different format because [...]. Therefore the two must exchange Game type data through an IPC mechanism”, or something among those lines. It is much more clear IMO. But I think that for it to work, the “A Bevy application (the Game) uses Rust types [...]” sentence should be simplified. In particular, I think there is no need to distinguish the origin of the types. It does not care (at least to a certain level) if they are defined inside the Project's crate/workspace or defined in a dependency. And I think, reflection data contains namespace info up to the crate name if we really need that info, and we get it for free.

Copy link
Author

Choose a reason for hiding this comment

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

The Editor and Game data models are of different format because [...]. Therefore the two must exchange Game type data through an IPC mechanism.

Nit-picking a bit here, but technically different formats is not a cause that implies IPC as a consequence. IPC is only for separate processes. Although I understand what you mean, assuming separate processes then IPC is a consequence of the formats being different.

Copy link
Author

Choose a reason for hiding this comment

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

Applied some of those suggestions, breaking up the user-facing explanation into sections and defining some missing terms.

rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
@djeedai
Copy link
Author

djeedai commented Jul 21, 2022

Thanks for the review @Nilirad, I'll have a look in details and address the comments. In the meantime, I wanted to react to this:

I strongly recommend you to use semantic line breaks, as they help a lot the reviewing process. Single-line paragraphs are not very ergonomic, and make it difficult to pinpoint an issue. Start by giving the entire line to each sentence, then if you feel like it, break sentences into their logical pieces. See sembr.org for more about semantic line breaks.

This is the first time I hear about this, so I've looked at that website with curiosity and interest. Unfortunately, that felt quite short of my expectations. So forgive me as I'm going to go on an off-topic rant against it: that "semantic line break" seems purely detrimental to me. Can you clarify why you think "they help a lot the reviewing process"? For me:

  • The reason I think we have a RENDERED at the top is so people can review on the final rendered text. Most IDEs, even online, have a preview mode. Maybe that's a personal preference, but I don't read/review markdown from the source; the very point of markdown is so we can have a rendered text with paragraphs and sentences that I can read like a regular book. Now you may argue you review from sources, and that's a fair argument but...

  • sembr.org goes into an illustrative example about adding line breaks to increase clarity. What they really do here is break lines so they fit into the screen space. Unfortunately for them, on mobile (where I looked at the link first) even the last block with the semantic breaks extends past the visible area (without wrapping!) and so has an horizontal scrollbar. Result: none of them are more readable than the other, as all of them are truncated without wrapping, and the example falls short of demonstrating anything. And there is no other explanation given as to why semantic line breaks would be beneficial; the example was supposed to be enough. What I personally get from it is that you should have text wrapping, or maybe do wrapping by hand if you're not sure the reader will have an IDE doing it. But their example shows doing it semantically doesn't work. I personally find it largely easier to read a text block wrapped at a fixed column size (say 80 char) than randomly at each sentence's end.

  • On the "reading from source", adding line breaks after sentences directly goes against the very thing that makes languages readable. There's already a construct to separate thoughts, and it's paragraphs.

    The function of a paragraph is to mark a pause, setting the paragraph apart from what precedes it.
    -- https://en.wikipedia.org/wiki/Paragraph

    If two lines are following each other without a break, this is because they are part of a "unit of thought" or idea that goes together. This whole idea of semantic line breaks just goes against that. I'm fine with rules like breaking down long sentences into shorter ones etc., which are helpful rules for writing easy-to-read texts (it's even proven), but putting each sentence on a separate line is not that.

  • Last but not least, I bet there's a very negative effect on reading speed, as your eyes are forced to scan back to the beginning of the next line at each sentence, and sentences are generally of quite varying lengths. Paragraphs in typography have attributes like indentation and spacing that guide the eye to the next paragraph. Here the proposed semantic line break throws all of that away.

Now, I would accept one argument, which is mentioned nowhere and is unrelated to semantic: line breaks might actually help in the context of version control, since VCSes like git are line-based, so adding a line break influences diffs and merges. But I'm not sure what's the extent of that influence, so I'd be wary to just add line breaks randomly.

@djeedai
Copy link
Author

djeedai commented Jul 21, 2022

Updated with @Nilirad's suggestions. Not sure what's the usual process for comments, should I leave open for you to confirm resolution, or should I mark Resolved myself when I applied some change?

@Nilirad
Copy link

Nilirad commented Jul 22, 2022

@djeedai, About semantic line breaks, I think you are mistaking how most markup languages are rendered. For most flavors of Markdown (GitHub comments and Discord excluded), a single line feed character (LF) does not result in a line feed or a new paragraph in the rendered HTML. To create a new paragraph you have to add at least two consecutive LF.

So, semantic line breaks do not affect the final output. This can be used at our advantage to improve the editing experience. For example, see the “Motivation” paragraph I reviewed a couple of days ago:

We need a data model for the Bevy Editor. We want to make it decoupled from the runtime data model that the Game uses, which is based on compiled Rust types (`World`, _etc._) with a defined native layout from a fixed Rust compiler version (as most types don't use `#[repr(C)]`), so we can handle data migration across Rust compiler versions and therefore across both Editor and Game versions. We also want to make it _transportable_, that is serializable with the intent to transport it over any IPC mechanism, so that the Editor process can dialog with the Game process to enable real-time Game editing ("Play Mode").

This is a single line of Markdown code. Since both VCS versioning and review comments are line-based, it is impossible to locate the thought I want to address without using quotes. It also forced me to stuff all the comments inside one single discussion. Giving each thought its own line instead will improve the reviewing experience:

We need a data model for the Bevy Editor.
We want to make it decoupled from the runtime data model that the Game uses,
which is based on compiled Rust types (`World`, _etc._)
with a defined native layout from a fixed Rust compiler version
(as most types don't use `#[repr(C)]`),
so we can handle data migration across Rust compiler versions
and therefore across both Editor and Game versions.
We also want to make it _transportable_,
that is serializable with the intent to transport it over any IPC mechanism,
so that the Editor process can dialog with the Game process
to enable real-time Game editing ("Play Mode").

The paragraph has been split into their different logical blocks, separating the cause from the effect, the premise from the consequence. In this way, each single thought can be referenced independently. As explained before, this does not change the HTML output, but it will improve writing (the physical structure of the text reflects its logical structure) and editing (most text edits are limited to a single thought, e.g. changing a wrong assumption about a logical consequence to the premise in the previous line).

For example, here I can already spot an issue with the paragraph: the speech does not flow well, because many thoughts are stuffed inside a single sentence, 5 in one case (not counting the parentheses), where a well constructed sentence is way shorter, with 3 or 4 thoughts at most.

EDIT: Re-reading your previous message, i think the word “thought” I used here is not completely appropriate. See them more like “logical blocks” of a sentence.

Not sure what's the usual process for comments, should I leave open for you to confirm resolution, or should I mark Resolved myself when I applied some change?

I think it depends. When a suggestion can be univocally applied, there is no reason to request feedback on that. In the other cases it is always better to wait for feedback.

Copy link

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

I'll try to furtherly restrict the scope of my reviews to make them more manageable. This time it's about the “Glossary” section. The order of the definitions looks correct (there are no cases of term use before definition) and there are no circular dependencies.

rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
Copy link

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

A couple of comments about the user-facing explanation.

rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
Copy link

@afonsolage afonsolage left a comment

Choose a reason for hiding this comment

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

Nice work! Later on I'll do another reading to understand even better the concepts, but I think it's in a good direction.

I'm really excited to see more progress on IPC and maybe debugging features!

One thing to note tho, I found kinda confusing the definition of GameObject and GameType. I don't think we should use the Object at all, since the Bevy Editor will use a full ECS architecture, and we should use Component, System, Entity and Resources whenever we can.

For instance GameType is a description of a GameComponentType and GameObject is the GameComponentInstance or something like that.

Also, as a final comment, I think a "Out of scope" section would be nice to avoid discussions that won't be covered by this RFC, like Systems which is another big piece of Bevy Editor and is out of scope of this RFC.

rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
Copy link

@afonsolage afonsolage left a comment

Choose a reason for hiding this comment

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

Added a separated review just to address Our Machinery broken links, to replace with web.archive ones.

rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
- Remove confusing "1:1 mapping" reference
- Add resource in intrinsic model types
- Rework the pseudo-code example
Copy link

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Stopping here since it's getting too long (and I'm starting to forget the initial comments I drafted a couple of weeks ago 😅)

Some general notes:

I would move the section about game objects before the Sub-type section, since there are many references to it.

I'm really struggling about understanding how entities in the Game world are represented in the data model. All I understand is that each GameObject is an instance of a GameType, which presumably is a representation of Game components, but I've seen nothing that indicates to which Game entity those Game components belong to. I feel that an overview at the beginning of the implementation strategy before going into the details, would be of much help to get the bigger picture.

I've seen in general that the term “object” seems to be a little bit too overloaded and makes it difficult to separate the different concepts. It is used throughout the RFC to refer to:

  1. Rust data structures (e.g. instances (allocated objects))
  2. reference to another [intrinsic] type.
  3. Game objects

I also have seen many instances where the “game” word is used as an identifier (e.g. GameObject, GameType, GameWorld). As often Cart states, this is not correct, since a Bevy application is not necessarily a game. So, sooner or later, we have to find a better name. Since the Editor-Game relationship is a server-client one, we could go with “client”, but I'm open for other alternatives.

I also think we should get in touch with bevy_reflect folks to discuss how the reflection groundwork can be laid out to make it editor-ready, but I would wait after the RFC is a little bit more polished.


### Built-in types

The data model supports a number of built-in types, corresponding to common Rust built-in types.
Copy link

Choose a reason for hiding this comment

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

It would be beneficial to add some context on why such Editor model built-in types are needed.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure how to justify that except by saying that I don't see how else you'd handle an i32 or a bool in the model. Surely you don't want to have a struct bool { }.


#### Arrays

Arrays are dynamically-sized homogenous collections of _elements_. The `ArrayType` defines the element type, which can be any valid game type.
Copy link

Choose a reason for hiding this comment

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

I would prefer the word “list“ or “vector” instead, since array is usually referred to statically sized data structures.

Copy link
Author

Choose a reason for hiding this comment

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

List often refers to a chained/linked list (except in C#...) and vector often refers to the Math type too. And array in C# is a vector. There's unfortunately no ubiquitous terminology when it comes to data structures in computer science. Here I was worried about the possible confusion with a Math vector.

Copy link

Choose a reason for hiding this comment

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

I did some quick research, and “array” by itself does not specify if it is static or dynamic. So I think the current terminology could be OK.

Choose a reason for hiding this comment

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

Since Array in bevy_reflect does refer to a fixed-size array I'd opt for List instead to match bevy_reflect.


### Built-in types

The data model supports a number of built-in types, corresponding to common Rust built-in types.
Copy link

Choose a reason for hiding this comment

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

We need to use different expressions identify built-in types (for the Editor data model) from the built-in types (from Rust).

I prefer to use “primitive type” when referring to the Rust types like u32, f64, etc.
Then I would not use “built-in type” since there's risk of it being synonimized with “primitive type”.

Copy link
Author

Choose a reason for hiding this comment

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

Base types? I really don't know how else to call them, they're literally the types built in the data model, that allow creating other types.

Copy link

Choose a reason for hiding this comment

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

Hmm, I think we can still call them built-in types, but we need to call Rust primitive types with their actual name to avoid confusion.

Suggested change
The data model supports a number of built-in types, corresponding to common Rust built-in types.
The data model supports a number of built-in types, corresponding to common Rust primitive types.


_Note_: We explicitly handle all integral and floating-point bit variants to ensure tight value packing and avoid wasting space both in structs (`GameObject`; see below) and collections (arrays, _etc._).

`ObjectRef` is a reference to any object, custom or built-in. This allows referencing components built-in inside Bevy or the Editor (TBD depending on design of that part). The reference needs to be valid (non-null) when the data gets processed for baking, but can temporarily (including while serialized to disk and later reloaded) be left invalid (`None`) for editing flexibility. Nullable references require a `NullableRef` component to be added to mark the reference as valid even if null.
Copy link

Choose a reason for hiding this comment

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

Suggested change
`ObjectRef` is a reference to any object, custom or built-in. This allows referencing components built-in inside Bevy or the Editor (TBD depending on design of that part). The reference needs to be valid (non-null) when the data gets processed for baking, but can temporarily (including while serialized to disk and later reloaded) be left invalid (`None`) for editing flexibility. Nullable references require a `NullableRef` component to be added to mark the reference as valid even if null.
`ObjectRef` is a reference to any object, custom or built-in. This allows referencing components built-in inside Bevy or the Editor (TBD depending on design of that part). A reference can be valid (`Some`) or invalid (`None`). When the data gets processed for baking, all `ObjectRef`s must be valid. Some `ObjectRef`s, called _nullable references_, can be temporarily left invalid for editing flexibility. Nullable references require a `NullableRef` component to be added to mark the reference as valid even if null.

I prefer this wording because I believe it clearly defines valid vs invalid, and when a reference can be left invalid. It also defines what nullable references are.

I also removed the “including while serialized to disk and later reloaded” text, since the mapping between the Editor data model and its serialized form can matemathically be seen as a bijective (and therefore invertible) function. Therefore, it's natural that every detail of the data model will be reflected into persistent storage and vice versa.

Copy link
Author

Choose a reason for hiding this comment

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

That rewording is good, except it changed the meaning. I will update.

struct NullableRef;
```

This allows filtering out nullable references, and collect invalid `ObjectRef` instances easily via a `Query`, for example to emit some warning to the user.
Copy link

Choose a reason for hiding this comment

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

Looks like you are referring to two different applications of NullableRef, but it's not super clear:

  1. Filtering out nullable references
  2. Warn about invalid ObjectRefs

Maybe expanding a little bit on (1) may give more contextual knowledge.

}
```

Properties themselves are defined by a name and the type of their value. The type also stores the byte offset of that property from the beginning of a struct instance, which allows packing property values for a struct into a single byte stream, and enables various optimizations regarding diff'ing/patching and serialization. Modifying any of the `name`, `value_type`, or `byte_offset` constitute a type change, which mandates migrating all existing instances.
Copy link

Choose a reason for hiding this comment

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

Modifying any of the name, value_type, or byte_offset constitute a type change, which mandates migrating all existing instances.

This feels out of place, I would put it under the “Migration rules” section.

Copy link
Author

Choose a reason for hiding this comment

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

Deleted.

Copy link

Choose a reason for hiding this comment

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

Fine. I think the current state of the Migration rules clearly express the migration conditions.

}
```

Properties themselves are defined by a name and the type of their value. The type also stores the byte offset of that property from the beginning of a struct instance, which allows packing property values for a struct into a single byte stream, and enables various optimizations regarding diff'ing/patching and serialization. Modifying any of the `name`, `value_type`, or `byte_offset` constitute a type change, which mandates migrating all existing instances.
Copy link

Choose a reason for hiding this comment

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

Does the Property type correspond to the “property” mentioned in the user-facing explanation? They seem to only apply to StructType and not other editor types.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it does, and that can only apply to StructType because that's the only composable type. Same as Rust really.

rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
rfcs/62-editor-data-model.md Outdated Show resolved Hide resolved
@viridia
Copy link

viridia commented Jul 20, 2023

So, before I begin, some context: I've written many game editors in my career, going all the way back to 1986. This includes editors I wrote while at Electronic Arts and Maxis. My most recent game engine / editor consists of 70k lines of TypeScript. So while my knowledge may be a bit out of date, I have a bit of experience in this realm.

The requirements given here have a number of shortcomings from my perspective.

The most important of these is the need to support custom types that are not just aggregates of standard types. In my current editor, there are about a dozen built-in editable types (float, int, bool, struct, vector3, etc.), but there are 24 custom field editors, that is, field editors that are specific to the game. One such example is the "transition predicate" type, which allows the artist to specify the conditions under which a quest transitions from one stage to another. Many custom types are context-specific, so for example while editing a dialogue tree, the "next state" field shows a dropdown list of the available states within the currently edited dialogue - not a global list of all possible states. Trying to edit this as a plain string would be an artist nightmare.

Also, many edit fields are closely integrated with the 3D view. For example, when editing a waypoint location field, a gizmo appears in the 3D window showing the location precisely. Trying to edit a location by typing in the coordinates is nearly impossible to get right.

Another function of the editor is to display important game data that is normally invisible - physics colliders, navigation meshes, waypoints, particle emitters and many others I could name. The artist needs to be able to enable and disable these overlays during the authoring process.

My current editor also supports a lot of game-specific editing modes, such as terrain sculpting, tree painting and biome painting. These are all plugins that are part of the general editing framework - the sculpting tools get mouse events just like any other ui widget, and handle the 3d picking via a set of common library methods.

On the other hand, some of the requirements listed here are non-goals from my perspective. I've never seen a case where an artist needed to update the game code in the middle of an editing session. Most of the editors I've worked with were a single binary that included both the game and the editor, and a typical artist workflow would pull a new version on a daily basis at most.

@nicopap
Copy link

nicopap commented Oct 26, 2023

I think this RFC is chewing a bit more than it can. I have more questions after reading the RFC than before.

  • It seems to try to invent the concept of dynamic query? With the GameType component…
  • But also an offset-based ReflectPath implementation?
  • And a serialization format?
  • And what's this story about "transactional access to the game state"? How does it relate to the "motivation" section?
  • You talk about using the game state to "populate a User Interface". Re-reading the "motivation" section, I feel like UI shouldn't be a concern.
  • Concerning implementation: I don't understand what "Names" do.
  • You talk about a marker component to identify the "source" of a state. But I thought the state was stored in a World distinct from the actual ECS. In any case, what are you trying to distinguish between? If the editor and game processes are separate, no need to mark components.

Otherwise it's fairly unclear what it is trying to accomplish. For one, the language is very formal, weirdly lawyerly, and difficult to read. It reads like the Facebook Terms of Service. No ones wants to read a ToS! Here is a very useful resource on writing: https://www.plainlanguage.gov/resources/articles/dash-writing-tips/. You should follow those instructions. eg: you should avoid rare words like "forfeiting", and use less often the passive voice.

Also the RFC should probably be trimmed. It talks about a lot of different things, but I don't understand how those things relate to each others.

@djeedai
Copy link
Author

djeedai commented Dec 18, 2023

The Motivation section has been rewritten to account for reviews. I will follow up on other sections too, to remove some tangential details and focus on the core purpose.

@djeedai djeedai marked this pull request as ready for review December 21, 2023 23:06
@MiniaczQ MiniaczQ mentioned this pull request Dec 22, 2023
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

8 participants