Details

Reviewers

Bastien Montagne (mont29)
Brecht Van Lommel (brecht)

Maniphest Tasks

T82819: Merge Asset Browser Milestone 1 (Not Necessarily Feature-Complete)

Commits

rBb71eb3a105b8: Asset System: Data-block asset metadata storage, reading and API

Summary

Asset meta-data is what turns a regular data-block into an asset.

The design foresees that asset data-blocks store a ID.asset_data pointer of type AssetMetaData. This data must not have dependencies on other data-blocks or data-block data, it must be an independent unit. That way we can read asset-metadata from .blends without reading anything else from the file.

Includes:

New ID.asset_data for asset metadata.
Asset tags, description and custom properties.
BKE code to manage asset meta-data and asset tags.
Code to read asset data from files, without reading IDs.
RNA for asset metadata (including tags)

Diff Detail

Repository: rB Blender

Event Timeline

Julian Eisel (Severin) created this revision.Dec 2 2020, 12:51 PM

Harbormaster completed remote builds in B11515: Diff 31530.Dec 2 2020, 12:52 PM

Julian Eisel (Severin) requested review of this revision.Dec 2 2020, 12:52 PM

Actually, I will split off the changes to create an asset (includes the UI ones) in a bit.

Remove UI changes, only include DNA, RNA + read/write changes now

Harbormaster completed remote builds in B11518: Diff 31534.Dec 2 2020, 4:03 PM

Julian Eisel (Severin) retitled this revision from Asset Browser: Metadata creation, storage and reading to Asset Browser: Metadata storage, reading and API.Dec 2 2020, 4:08 PM

Julian Eisel (Severin) edited the summary of this revision. (Show Details)

Julian Eisel (Severin) added projects: Pipeline, Assets & I/O, Asset Browser (Archived) (Milestone 1: Basic, Local Asset Browser).

Julian Eisel (Severin) added a task: T82819: Merge Asset Browser Milestone 1 (Not Necessarily Feature-Complete).Dec 2 2020, 4:15 PM

Asset Browser: "Make Asset" operator and UI integration

Harbormaster completed remote builds in B11519: Diff 31536.Dec 2 2020, 5:17 PM

Undo previous update

Grrr.. Arc updated this patch instead of creating a new one. Sorry for the noise.

Harbormaster completed remote builds in B11520: Diff 31537.Dec 2 2020, 5:19 PM

Looks good from my side.

This revision is now accepted and ready to land.Dec 2 2020, 7:39 PM

Julian Eisel (Severin) mentioned this in T82819: Merge Asset Browser Milestone 1 (Not Necessarily Feature-Complete).Dec 2 2020, 11:12 PM

Generally looks fine, but at least the padding in DNA needs to be addressed before this lands in master imho.

source/blender/blenkernel/intern/asset.c
105	Maybe should assert that the given tag is indeed into the tags list?
source/blender/blenloader/BLO_readfile.h
135	`tot_items` or something like that?
source/blender/blenloader/intern/readfile.c
972	`> -1` ? Again, `0` is potentially a valid value...
3636	This is going to read the whole data-block data, why not rather have something similar to what we do for previews? See `BLO_blendhandle_get_previews`. This would enforce/assume some layout of asset data on file, but on the other end it avoids reading and allocating memory for the whole datablock (and its sub-data).
source/blender/blenloader/intern/readfile.h
116	Is this true? `DNA_elem_offset` returns `-1` when it cannot find the element.
source/blender/makesdna/DNA_text_types.h
64 ↗	(On Diff #31537)	This is 100% wrong. You should never have to modify any ID type because you modify the 'parent' `ID` struct. Think here issue is that you get an un-even number of pointers in `ID`, so that's where you need to add the `void *_pad_v1`?
source/blender/makesrna/intern/rna_asset.c
130	This is fairly inefficient... Wouldn't it be simpler/cleaner to store the amount of tags in an integer in `AssetMetaData`? You have room there anyway within the padding currently.

This revision now requires changes to proceed.Dec 3 2020, 5:20 PM

Address review requests

Harbormaster completed remote builds in B11553: Diff 31583.Dec 3 2020, 7:37 PM

Julian Eisel (Severin) added inline comments.Dec 3 2020, 7:40 PM

source/blender/blenloader/intern/readfile.c
3636	Asset data has custom properties, which I think are a party pooper here, but are important. I guess they are stored as data after the ID? So we have no choice but reading that I guess. But maybe we could have a smarter version of `read_data_into_datamap()` that only reads data of a set of given SDNA numbers.
source/blender/makesrna/intern/rna_asset.c
130	Kinda annoying to have such easily query-able runtime info stored. We could add an `AssetMetaData_RuntimeInfo` and populate that on read. But we can still do that later, it wouldn't break compatibility. Just added the count to the struct for now.

besides note in RNA 9which is trivial to fix), patch LGTM.

source/blender/blenloader/intern/readfile.c
3636	Not sure how/why IDProps would be an issue here? they will be written immediately after `AssetMetaData` struct, together with the `ListBase` of tags and their items. So basically, you'd just need to loop over all `bhead`s after the ID one, until you find the `bhead->SDNAnr == DNA_struct_find_nr(fd->filesdna, "AssetMetaData")` one, then read all the consecutive bheads of `IDProperty`, `ListBase` and `AssetTag` types, take care of remapping pointers yourself, and you are done? Not saying this is needed for this patch to land in master, but imho that's something you want in the long run.
source/blender/makesrna/intern/rna_asset.c
130	Adding the count in the struct is nice, but would be nicer to actually use it here? ;)

This revision is now accepted and ready to land.Dec 4 2020, 10:26 AM

If nobody objects - I'll also go ahead and rename tags to keywords. That has a less technical framing, and I've seen a number of other apps use that term too.

In D9716#242990, @Julian Eisel (Severin) wrote:

If nobody objects - I'll also go ahead and rename tags to keywords. That has a less technical framing, and I've seen a number of other apps use that term too.

I'd object, for me tag makes much more sense than keyword here? What are the app using keyword, and in which context?

To me "tag" seems more common and I'd expect it to be understood better by users. I'm not sure that "keyword" is less technical?
https://en.wikipedia.org/wiki/Tag_(metadata)

"Keyword" is definitely a common alias for tags, for example it is used by other 3D DCC apps for asset management.

Another thing, currently linked data-blocks can be made assets. Although in testing, this worked fine, but I think we should forbid this at first. It's something we didn't discuss much and I don't want to risk a can of worms.

I'd have to agree with others that tag is a better word here. Here's one argument:

"Keyword" sort of implies a text based context. For example, if you're searching for research papers about a topic, "keyword" makes sense, because you're literally searching for "key" "words" from the text.
But to me it makes less sense than "tag" for visual things that aren't even made up of words.

It also just sounds more natural to me for this situation.

It also depends what the intended purpose of tags is. If it's only to help search and filtering, then keywords can work.

If it's also to e.g. indicate to an automated tool that some asset is e.g. a prop / character / shot asset, then I don't think it's the right term. But that can (and perhaps should be) done with custom properties.

Still I think tag is fine as a term, I don't see much reason to change it.

Another thing, currently linked data-blocks can be made assets. Although in testing, this worked fine, but I think we should forbid this at first. It's something we didn't discuss much and I don't want to risk a can of worms.

I'm not sure how this can work, the metadata would be lost on save and reload?

Storing the kind of an asset (char, set, prop, ...) should indeed be done as a separate property I think, not using tags.
I still prefer "keywords" but I'm not motivated enough to have that argument. So I will leave it at "tags".

Another thing, currently linked data-blocks can be made assets. Although in testing, this worked fine, but I think we should forbid this at first. It's something we didn't discuss much and I don't want to risk a can of worms.

I'm not sure how this can work, the metadata would be lost on save and reload?

Only briefly tested it a bit ago, testing again now, yes the asset metadata is lost on reload. I will disable the asset operators for linked IDs.

Julian Eisel (Severin) mentioned this in T83674: Avoid reading of all ID data when reading asset data.Dec 11 2020, 6:02 PM

Julian Eisel (Severin) marked an inline comment as done.Dec 11 2020, 6:13 PM

Julian Eisel (Severin) added inline comments.

source/blender/blenloader/intern/readfile.c
3636	Thinking about this further, yeah I guess if we can assume ID props (including nested ones) to write right after `AssetMetaData` - which I think we can - then this should work I guess. Added TODO (not necessarily for 2.92): T83674
source/blender/makesrna/intern/rna_asset.c
130	Yikes. Also should `MAX2(.., 0)` it. Fixed locally.

Closed by commit rBb71eb3a105b8: Asset System: Data-block asset metadata storage, reading and API (authored by Julian Eisel (Severin)). · Explain WhyDec 15 2020, 5:04 PM

This revision was automatically updated to reflect the committed changes.

Julian Eisel (Severin) marked an inline comment as done.

Julian Eisel (Severin) added a commit: rBb71eb3a105b8: Asset System: Data-block asset metadata storage, reading and API.

Asset Browser: Metadata storage, reading and API
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 32014

source/blender/CMakeLists.txt

source/blender/blenkernel/BKE_asset.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/asset.c

source/blender/blenkernel/intern/lib_id.c

source/blender/blenkernel/intern/lib_id_delete.c

source/blender/blenloader/BLO_readfile.h

source/blender/blenloader/intern/readblenentry.c

source/blender/blenloader/intern/readfile.h

source/blender/blenloader/intern/readfile.c

source/blender/makesdna/DNA_ID.h

source/blender/makesdna/DNA_asset_defaults.h

source/blender/makesdna/DNA_asset_types.h

source/blender/makesdna/intern/CMakeLists.txt

source/blender/makesdna/intern/dna_defaults.c

source/blender/makesdna/intern/makesdna.c

source/blender/makesrna/RNA_access.h

source/blender/makesrna/intern/CMakeLists.txt

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_ID.c

source/blender/makesrna/intern/rna_asset.c

source/blender/makesrna/intern/rna_internal.h

Asset Browser: Metadata storage, reading and APIClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 32014

source/blender/CMakeLists.txt

source/blender/blenkernel/BKE_asset.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/asset.c

source/blender/blenkernel/intern/lib_id.c

source/blender/blenkernel/intern/lib_id_delete.c

source/blender/blenloader/BLO_readfile.h

source/blender/blenloader/intern/readblenentry.c

source/blender/blenloader/intern/readfile.h

source/blender/blenloader/intern/readfile.c

source/blender/makesdna/DNA_ID.h

source/blender/makesdna/DNA_asset_defaults.h

source/blender/makesdna/DNA_asset_types.h

source/blender/makesdna/intern/CMakeLists.txt

source/blender/makesdna/intern/dna_defaults.c

source/blender/makesdna/intern/makesdna.c

source/blender/makesrna/RNA_access.h

source/blender/makesrna/intern/CMakeLists.txt

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_ID.c

source/blender/makesrna/intern/rna_asset.c

source/blender/makesrna/intern/rna_internal.h

Asset Browser: Metadata storage, reading and API
ClosedPublic
Actions

Revision Contents
Changeset List