blender/blender-developer-docs
7
8
Fork 38
Code Issues 3 Pull Requests 6 Activity

ID: Document the steps to properly add a new ID data-block type #15

Open
Sybren A. Stüvel wants to merge 1 commits from pr/new-id-datablock into main
pull from: pr/new-id-datablock
merge into: blender:main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Conversation 8 Commits 1 Files Changed 1 +45 -2
Sybren A. Stüvel commented 2024-02-05 17:16:35 +01:00
Member
Copy Link

Extend the existing documentation of how to add a new IDTypeInfo, so that
it includes all the places in which the new ID datatype has to be added
before Blender truely can work with it.

This is based on what I had to do for the new Animation data-block
introduced in blender/blender#114098.

Extend the existing documentation of how to add a new `IDTypeInfo`, so that it includes all the places in which the new ID datatype has to be added before Blender truely can work with it. This is based on what I had to do for the new `Animation` data-block introduced in https://projects.blender.org/blender/blender/pulls/114098.
👍 1
Sybren A. Stüvel added 1 commit 2024-02-05 17:16:36 +01:00 
Extend the existing documentation of how to add a new `IDTypeInfo`, so that
it includes all the places in which the new ID datatype has to be added
before Blender truely can work with it.

This is based on what I had to do for the new `Animation` data-block
introduced in #114098.
Sybren A. Stüvel requested review from Bastien Montagne 2024-02-05 17:16:42 +01:00
Damien Picard reviewed 2024-02-05 17:27:17 +01:00
docs/features/core/datablocks/id_type.md
@ -188,0 +197,4 @@
- `which_libbase()`
- `set_listbasepointers()`
6. `readfile.c`: in `dataname()`
7. `BLT_translation.h`: add a `#define BLT_I18NCONTEXT_ID...` line
Damien Picard commented 2024-02-05 17:27:17 +01:00
Member
Copy Link
  1. BLT_translation.h: add #define BLT_I18NCONTEXT_ID... and BLT_I18NCONTEXTS_ITEM(BLT_I18NCONTEXT_ID_..., "id_..."), \ lines
7. `BLT_translation.h`: add `#define BLT_I18NCONTEXT_ID...` and `BLT_I18NCONTEXTS_ITEM(BLT_I18NCONTEXT_ID_..., "id_..."), \` lines
dr.sybren marked this conversation as resolved
Falk David requested changes 2024-02-05 17:29:20 +01:00
Falk David left a comment
Member
Copy Link

I didn't try to render this locally, but rendering this on gitea leads to some inconsistent indentation. Looks like previous lines in this file used 4 spaces for indentation and this is unsing only 2.

I didn't try to render this locally, but rendering this on gitea leads to some inconsistent indentation. Looks like previous lines in this file used 4 spaces for indentation and this is unsing only 2.
docs/features/core/datablocks/id_type.md
@ -188,0 +190,4 @@
- Add a new `case` to `BKE_idtype_idcode_from_index()`
Additionally, there are some other areas in the source code where a reference to
the new type might be added:
Falk David commented 2024-02-05 17:24:22 +01:00
Member
Copy Link

might be added -> might be needed ?

`might be added` -> `might be needed` ?
dr.sybren marked this conversation as resolved
docs/features/core/datablocks/id_type.md
@ -188,0 +196,4 @@
5. `blenkernel/intern/main.cc`:
- `which_libbase()`
- `set_listbasepointers()`
6. `readfile.c`: in `dataname()`
Falk David commented 2024-02-05 17:23:09 +01:00
Member
Copy Link

This is readfile.cc now.

This is `readfile.cc` now.
dr.sybren marked this conversation as resolved
Sybren A. Stüvel commented 2024-02-05 17:48:16 +01:00
Author
Member
Copy Link

Thanks for the feedback!

I didn't try to render this locally, but rendering this on gitea leads to some inconsistent indentation. Looks like previous lines in this file used 4 spaces for indentation and this is unsing only 2.

I tried indenting with 4 spaces earlier, but then the nested lists didn't work properly. mkdocs seems to be quite sensitive to the exact amount of whitespace, unfortunately.

Thanks for the feedback! > I didn't try to render this locally, but rendering this on gitea leads to some inconsistent indentation. Looks like previous lines in this file used 4 spaces for indentation and this is unsing only 2. I tried indenting with 4 spaces earlier, but then the nested lists didn't work properly. mkdocs seems to be quite sensitive to the exact amount of whitespace, unfortunately.
Sybren A. Stüvel requested review from Falk David 2024-02-05 17:48:43 +01:00
Julian Eisel commented 2024-02-07 14:00:21 +01:00
Member
Copy Link

I'm wondering if we really want to go into that much detail, this is prone to getting outdated in no time. Then again, it might actually avoid some bugs. (I mean, I hope small documentation edits when changing code will become the norm, but at this point it's not common practice yet.)
Let's get some opinions. @brecht @Sergey what do you think?

I'm wondering if we really want to go into that much detail, this is prone to getting outdated in no time. Then again, it might actually avoid some bugs. (I mean, I hope small documentation edits when changing code will become the norm, but at this point it's not common practice yet.) Let's get some opinions. @brecht @Sergey what do you think?
Sergey Sharybin commented 2024-02-07 14:21:42 +01:00
Owner
Copy Link

Those areas do not change that often, and it is a topic which people keeps running into, and which is easy to make a mistake in. So I think it is ok to have such instruction.

We should also be able to leverage compiler warning much better. For example, ensure we do not silence any unhandled case in a switch statements in the areas where it is important. And by moving more things into the IDType info. For example, it would be great if we can more DEG building logic of various IDs away from the core DEG logic. But that's for the future work, not as part of this PR.

Those areas do not change that often, and it is a topic which people keeps running into, and which is easy to make a mistake in. So I think it is ok to have such instruction. We should also be able to leverage compiler warning much better. For example, ensure we do not silence any unhandled case in a switch statements in the areas where it is important. And by moving more things into the IDType info. For example, it would be great if we can more DEG building logic of various IDs away from the core DEG logic. But that's for the future work, not as part of this PR.
Brecht Van Lommel commented 2024-02-07 14:49:26 +01:00
Owner
Copy Link

I'd put this on its own page, and maybe at the beginning add some text like.

At the time of writing, these are the places that need to be modified.
Additionally, find a similar ID type and go through the places where it is used to verify and find more places where it might need to be integrated.

I'd put this on its own page, and maybe at the beginning add some text like. > At the time of writing, these are the places that need to be modified. > Additionally, find a similar ID type and go through the places where it is used to verify and find more places where it might need to be integrated.
👍 2
Iliya Katushenock commented 2024-02-07 17:27:58 +01:00
Member
Copy Link

Shouldn't list of new ID elements be added in bmain structure?

Shouldn't list of new ID elements be added in bmain structure?
👍 1
Bastien Montagne commented 2024-02-07 18:24:38 +01:00
Owner
Copy Link

TL;DR:

While getting better documentation about new ID type creation is indeed great, I'm not a big fan of such detailed instructions. The relevant code may not change that often, but imho it changes often enough that keeping this doc up to date with all the specifics will become a challenge. I'd rather have some more generic instructions.

Issues I see in this PR

I find giving a 'complete' recipe to be 'dangerous', in the sense that devs may stop thinking by themselves and just apply it blindly. Especially if there is no structure/explanation about the 'why' of each of these steps.

Further more, it's very hard to ensure that said recipe is actually complete. I do not see a reference to adding a new list of IDs in the Main struct in proposed patch e.g.

Some instructions in the proposed PR also do not belong here imho. E.g. under the RNA category, a lot is actually valid for any RNA editing, and should therefore be documented in the RNA section of the doc. Same goes for the handling of DNA 'default' headers.

Proposal

Like @brecht, I would put forward the fact that the best way to find which code needs to be edited when adding a new ID type is by looking for usages/references to other existing similar ID types (both the IDType_ID_... declared in BKE_idtype.hh, and the ID_... in DNA_ID_enums.h).

As @Sergey noted, a remark about using IDType switches (without default clause) and the fact that they will raise errors at compile time if one of the enum value is not handled is another nice 'generic' way to find missing handling cases.

And I would personally much rather see a list of 'areas of code' that do (or may) need some work, than specific references to files and functions. Maybe organized by 'new ID type creation' tasks, in some form of logical progression (from the most low-level, basic things to do in absolutely all cases, to less critical areas, and things that may be ignored in some cases):

  • ID type itself:
    • new IDTypeInfo in BKE_idtype, declaration, registering.
    • Implement needed callbacks for the ID type.
    • Update various utils like the conversion helpers (BKE_idtype_idcode_to_idfilter etc.).
  • DNA:
    • Regular DNA editing applies (about adding new DNA files, handling 'default' values for structs, etc.) (should link to DNA doc)
    • new ID 'derived' struct (likely in a new DNA header), and its sub-data structs if needed.
    • Add new defines and enum values:
      • in ID_Type itself (DNA_ID_enums.h);
      • define a FILTER_ID_, and add it to FILTER_ID_ALL (DNA_ID.h).
      • add a eID_Index value (DNA_ID.h).
  • Main:
    • Add list for new ID type in Main struct.
    • update BKE_main code (which_libbase etc.).
  • ID management:
    • Most of it should now be handled by the callbacks in the new IDTypeInfo, however some aspects are not yet fully ported to it.
    • BKE_lib_query (e.g. BKE_library_id_can_use_filter_id)
  • Depgraph:
    • Currently depsgraph building and evaluation logic is still centralized in the DEG code. deg_builder_relations and deg_builder_nodes.
  • RNA:
    • Regular RNA editing applies (about adding new RNA files and new RNA structs definitions and API). (should link to RNA doc)
    • Add a new call to RNA_MAIN_LISTBASE_FUNCS_DEF to enable the access to the Main's new ListBase by the new RNA collection.
    • Add a new call to RNA_MAIN_ID_TAG_FUNCS_DEF to enable the tagging of the IDs of the new type.
    • Add a new RNA_def_main_ function in rna_main_api, which defines the basic handling of the new RNA collection (like adding or removing an ID). And the related static functions (note that some functions are generated by macros and do not need to be defined explicitly, e.g. the tagging one through RNA_MAIN_ID_TAG_FUNCS_DEF).
    • Add a new RNA collection definition to the list of MainCollectionDef in RNA_def_main. It will wrap the matching new ListBase added to the Main struct, with the help of the functions and macro calls above.
  • UI/editors:
    • Add a new icon (and its definition in UI_icons.hh), update UI_icon_from_idcode
    • space_outliner:
      • Add a new entry to TREESTORE_ID_TYPE.
      • The outliner tree represent IDs as TreeElementID or some subclass.
        • TreeElementID::create_from_id at least needs to be updated.
        • If some specific behavior is needed, some sub-class of TreeElementID may be defined, in a new set of files (like e.g. tree_element_id_object.hh/.cc, etc.).
    • There are several other places in UI code handling ID types, like opengl_render.cc (gather_frames_to_render_for_id), or interface_templates.cc (template_id_browse_tip).
    • Add a new BLT_I18NCONTEXT_ID_ translation context in BLT_translation.
  • BLO:
    • readfile (dataname()) (sic)

Note that this is still a very rough layout idea, would need to be fleshed out, cleaned up...

### TL;DR: While getting better documentation about new ID type creation is indeed great, I'm not a big fan of such detailed instructions. The relevant code may not change that often, but imho it changes often enough that keeping this doc up to date with all the specifics will become a challenge. I'd rather have some more generic instructions. ### Issues I see in this PR I find giving a 'complete' recipe to be 'dangerous', in the sense that devs may stop thinking by themselves and just apply it blindly. Especially if there is no structure/explanation about the 'why' of each of these steps. Further more, it's _very_ hard to ensure that said recipe is actually complete. I do not see a reference to adding a new list of IDs in the Main struct in proposed patch e.g. Some instructions in the proposed PR also do not belong here imho. E.g. under the RNA category, a lot is actually valid for any RNA editing, and should therefore be documented in the RNA section of the doc. Same goes for the handling of DNA 'default' headers. ### Proposal Like @brecht, I would put forward the fact that the best way to find which code needs to be edited when adding a new ID type is by looking for usages/references to other existing similar ID types (both the `IDType_ID_...` declared in `BKE_idtype.hh`, and the `ID_...` in `DNA_ID_enums.h`). As @Sergey noted, a remark about using `IDType` switches (without `default` clause) and the fact that they will raise errors at compile time if one of the enum value is not handled is another nice 'generic' way to find missing handling cases. And I would personally much rather see a list of 'areas of code' that do (or may) need some work, than specific references to files and functions. Maybe organized by 'new ID type creation' tasks, in some form of logical progression (from the most low-level, basic things to do in absolutely all cases, to less critical areas, and things that may be ignored in some cases): * ID type itself: * new IDTypeInfo in BKE_idtype, declaration, registering. * Implement needed callbacks for the ID type. * Update various utils like the conversion helpers (`BKE_idtype_idcode_to_idfilter` etc.). * DNA: * Regular DNA editing applies (about adding new DNA files, handling 'default' values for structs, etc.) _(should link to DNA doc)_ * new ID 'derived' struct (likely in a new DNA header), and its sub-data structs if needed. * Add new defines and enum values: * in `ID_Type` itself (`DNA_ID_enums.h`); * define a `FILTER_ID_`, and add it to `FILTER_ID_ALL` (`DNA_ID.h`). * add a `eID_Index` value (`DNA_ID.h`). * Main: * Add list for new ID type in `Main` struct. * update BKE_main code (`which_libbase` etc.). * ID management: * Most of it should now be handled by the callbacks in the new IDTypeInfo, however some aspects are not yet fully ported to it. * BKE_lib_query (e.g. `BKE_library_id_can_use_filter_id`) * Depgraph: * Currently depsgraph building and evaluation logic is still centralized in the DEG code. `deg_builder_relations` and `deg_builder_nodes`. * RNA: * Regular RNA editing applies (about adding new RNA files and new RNA structs definitions and API). _(should link to RNA doc)_ * Add a new call to `RNA_MAIN_LISTBASE_FUNCS_DEF` to enable the access to the Main's new `ListBase` by the new RNA collection. * Add a new call to `RNA_MAIN_ID_TAG_FUNCS_DEF` to enable the tagging of the IDs of the new type. * Add a new `RNA_def_main_` function in `rna_main_api`, which defines the basic handling of the new RNA collection (like adding or removing an ID). And the related static functions (note that some functions are generated by macros and do not need to be defined explicitly, e.g. the tagging one through `RNA_MAIN_ID_TAG_FUNCS_DEF`). * Add a new RNA collection definition to the list of `MainCollectionDef` in `RNA_def_main`. It will wrap the matching new ListBase added to the Main struct, with the help of the functions and macro calls above. * UI/editors: * Add a new icon (and its definition in `UI_icons.hh`), update `UI_icon_from_idcode` * `space_outliner`: * Add a new entry to `TREESTORE_ID_TYPE`. * The outliner tree represent IDs as `TreeElementID` or some subclass. * `TreeElementID::create_from_id` at least needs to be updated. * If some specific behavior is needed, some sub-class of `TreeElementID` may be defined, in a new set of files (like e.g. `tree_element_id_object.hh`/`.cc`, etc.). * There are several other places in UI code handling ID types, like `opengl_render.cc` (`gather_frames_to_render_for_id`), or `interface_templates.cc` (`template_id_browse_tip`). * Add a new `BLT_I18NCONTEXT_ID_` translation context in BLT_translation. * BLO: * readfile (`dataname()`) (_sic_) Note that this is still a very rough layout idea, would need to be fleshed out, cleaned up...
👍 1
Bastien Montagne commented 2024-02-07 19:13:12 +01:00
Owner
Copy Link

PS: blender/blender!117958 should 'fix' the readfile case by removing any need to edit it when adding a new ID type.

PS: blender/blender!117958 should 'fix' the `readfile` case by removing any need to edit it when adding a new ID type.
Sybren A. Stüvel commented 2024-02-08 09:54:48 +01:00
Author
Member
Copy Link

@JulianEisel wrote:
I'm wondering if we really want to go into that much detail, this is prone to getting outdated in no time.

@mont29 wrote:
While getting better documentation about new ID type creation is indeed great, I'm not a big fan of such detailed instructions.

The large number of "oh I had to add something there too" that I had to go through was frustrating and time-consuming. These instructions were born from that frustration. I found the current documentation, of just three steps to add a new IDTypeInfo, to be misleading, as once that's done, that ID type is not actually usable in Blender.

I think the better approach here is not to avoid this level of detail in documentation, but rather write our software so that there is less entanglement. Having to hunt through basically all of Blender to add yet another "this is the icon for this type" or something similar.

Big note: I'm not blaming anyone, I'm just voicing frustration about the current state of things. Having an IDTypeInfo struct was a good idea, and IMO we should look at expanding this system, rather than adding yet more switch(GS(id->name)) or ELEM(GS(id->name), ID_...). Or have some way where "lists of ID-type-dependent things" can be checked against the known IDTypeInfo structs, so that at least at runtime (in debug builds) a warning can be emitted that that list is incomplete.

I find giving a 'complete' recipe to be 'dangerous', in the sense that devs may stop thinking by themselves and just apply it blindly. Especially if there is no structure/explanation about the 'why' of each of these steps.

I agree that there should be more structure & explanation.

Further more, it's very hard to ensure that said recipe is actually complete. I do not see a reference to adding a new list of IDs in the Main struct in proposed patch e.g.

And this is why the code should be better structured & commented in the first place. And why this documentation is essential to have. While writing the code, it's hard to know where to add what, as it's so all over the place. The fact that I didn't catch all places, but things still seem to work, is proof of that. Without this documentation, and a sense of "yeah now I'm done", developers will also at some point stop thinking for themselves, and just look for yet-another-thing-to-add when what they did so far doesn't work.

Some instructions in the proposed PR also do not belong here imho. E.g. under the RNA category, a lot is actually valid for any RNA editing, and should therefore be documented in the RNA section of the doc. Same goes for the handling of DNA 'default' headers.

That's fine by me, that's what we have links for.

Proposal

Like @brecht, I would put forward the fact that the best way to find which code needs to be edited when adding a new ID type is by looking for usages/references to other existing similar ID types (both the IDType_ID_... declared in BKE_idtype.hh, and the ID_... in DNA_ID_enums.h).

I'm fine adding this as some extra documentation, as it's simply good advice. I don't think it can replace a concrete list of places/areas to adjust, though.

As @Sergey noted, a remark about using IDType switches (without default clause) and the fact that they will raise errors at compile time if one of the enum value is not handled is another nice 'generic' way to find missing handling cases.

💯 but also there are many other places where things need to be added to lists that don't get caught by this.

And I would personally much rather see a list of 'areas of code' that do (or may) need some work, than specific references to files and functions. Maybe organized by 'new ID type creation' tasks, in some form of logical progression (from the most low-level, basic things to do in absolutely all cases, to less critical areas, and things that may be ignored in some cases):

👍

Note that this is still a very rough layout idea, would need to be fleshed out, cleaned up...

I'll see how clean I can get ;-)

> @JulianEisel wrote: > I'm wondering if we really want to go into that much detail, this is prone to getting outdated in no time. > @mont29 wrote: > While getting better documentation about new ID type creation is indeed great, I'm not a big fan of such detailed instructions. The large number of "oh I had to add something **there** too" that I had to go through was frustrating and time-consuming. These instructions were born from that frustration. I found the current documentation, of just three steps to add a new `IDTypeInfo`, to be misleading, as once that's done, that ID type is not actually usable in Blender. I think the better approach here is not to avoid this level of detail in documentation, but rather write our software so that there is less entanglement. Having to hunt through basically all of Blender to add yet another "this is the icon for this type" or something similar. Big note: I'm not blaming anyone, I'm just voicing frustration about the current state of things. Having an `IDTypeInfo` struct was a good idea, and IMO we should look at expanding this system, rather than adding yet more `switch(GS(id->name))` or `ELEM(GS(id->name), ID_...)`. Or have some way where "lists of ID-type-dependent things" can be checked against the known `IDTypeInfo` structs, so that at least at runtime (in debug builds) a warning can be emitted that that list is incomplete. > I find giving a 'complete' recipe to be 'dangerous', in the sense that devs may stop thinking by themselves and just apply it blindly. Especially if there is no structure/explanation about the 'why' of each of these steps. I agree that there should be more structure & explanation. > Further more, it's _very_ hard to ensure that said recipe is actually complete. I do not see a reference to adding a new list of IDs in the Main struct in proposed patch e.g. And this is why the code should be better structured & commented in the first place. And why this documentation is essential to have. While writing the code, it's hard to know where to add what, as it's *so* all over the place. The fact that I didn't catch all places, but things still seem to work, is proof of that. Without this documentation, and a sense of "yeah now I'm done", developers will *also* at some point stop thinking for themselves, and just look for yet-another-thing-to-add when what they did so far doesn't work. > Some instructions in the proposed PR also do not belong here imho. E.g. under the RNA category, a lot is actually valid for any RNA editing, and should therefore be documented in the RNA section of the doc. Same goes for the handling of DNA 'default' headers. That's fine by me, that's what we have links for. > ### Proposal > > Like @brecht, I would put forward the fact that the best way to find which code needs to be edited when adding a new ID type is by looking for usages/references to other existing similar ID types (both the `IDType_ID_...` declared in `BKE_idtype.hh`, and the `ID_...` in `DNA_ID_enums.h`). I'm fine adding this as some extra documentation, as it's simply good advice. I don't think it can replace a concrete list of places/areas to adjust, though. > As @Sergey noted, a remark about using `IDType` switches (without `default` clause) and the fact that they will raise errors at compile time if one of the enum value is not handled is another nice 'generic' way to find missing handling cases. 💯 but also there are many other places where things need to be added to lists that don't get caught by this. > And I would personally much rather see a list of 'areas of code' that do (or may) need some work, than specific references to files and functions. Maybe organized by 'new ID type creation' tasks, in some form of logical progression (from the most low-level, basic things to do in absolutely all cases, to less critical areas, and things that may be ignored in some cases): :+1: > Note that this is still a very rough layout idea, would need to be fleshed out, cleaned up... I'll see how clean I can get ;-)
👍 2
Bastien Montagne requested changes 2024-02-12 10:41:36 +01:00
Bastien Montagne left a comment
Owner
Copy Link

Removing from own review queue ;)

Removing from own review queue ;)
Falk David refused to review 2024-09-13 13:03:28 +02:00
Merge conflict checking is in progress. Try again in few moments.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin pr/new-id-datablock:pr/new-id-datablock
git checkout pr/new-id-datablock
Sign in to join this conversation.
Reviewers
No reviewers
Falk David
Bastien Montagne
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
8 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender-developer-docs#15
No description provided.
Delete Branch "pr/new-id-datablock"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?