RNA Default Values as Defines #32894

Closed
opened 2012-10-16 17:40:13 +02:00 by Harley Acheson · 18 comments
Member

%%%This patch can replace #32862, #32859, and #32854.

On almost all UI elements we can right-click and select "Reset to Default Value".
But this almost never resets it to default but instead sets it to the lowest value.
This is because almost no properties have default values set.

This patch adds more than 200 defaults to properties so that you can change many int,
float, boolean, or enum and then restore the default values with the right-click option.

As per Campbell Barton's request all defaults are defines within a single file, DNA_defaults.c%%%

%%%This patch can replace #32862, #32859, and #32854. On almost all UI elements we can right-click and select "Reset to Default Value". But this almost never resets it to default but instead sets it to the lowest value. This is because almost no properties have default values set. This patch adds more than 200 defaults to properties so that you can change many int, float, boolean, or enum and then restore the default values with the right-click option. As per Campbell Barton's request all defaults are defines within a single file, DNA_defaults.c%%%
Author
Member

Changed status to: 'Open'

Changed status to: 'Open'
Author
Member

%%%typo: All the defines are in a single file, DNA_defaults.h

%%%

%%%typo: All the defines are in a single file, DNA_defaults.h %%%

%%%I think the defines are a good idea, but I would just place them in the DNA file where the relevant structs are defined. And they should be used in blenkernel/ as well when the structs are created, as I understand it the whole point of have these defined in DNA is to ensure there is no duplication with that code.%%%

%%%I think the defines are a good idea, but I would just place them in the DNA file where the relevant structs are defined. And they should be used in blenkernel/ as well when the structs are created, as I understand it the whole point of have these defined in DNA is to ensure there is no duplication with that code.%%%
Author
Member

%%%Thanks Brecht! Will take a look and see how much work is required
to do it that way, but it might be more than I can find. Will see...%%%

%%%Thanks Brecht! Will take a look and see how much work is required to do it that way, but it might be more than I can find. Will see...%%%
Author
Member

%%%The attached new patch, DefaultsAsDefines2.diff, should be as Brecht asked for it:

There is no longer a DNA_Defaults.h file. Instead the defines are in the DNA files where the structs are defined.

Blenkernel files are also modified so the defaults are used when the structs are created.

this makes for a large diff file, at 3649 lines, affecting a lot of files. This is not ALL the defaults, as that would take until the end of time, just the most visible ones on all the major panels except for material and textures.

%%%

%%%The attached new patch, DefaultsAsDefines2.diff, should be as Brecht asked for it: There is no longer a DNA_Defaults.h file. Instead the defines are in the DNA files where the structs are defined. Blenkernel files are also modified so the defaults are used when the structs are created. this makes for a large diff file, at 3649 lines, affecting a lot of files. This is not ALL the defaults, as that would take until the end of time, just the most visible ones on all the major panels except for material and textures. %%%
Author
Member

%%%Although I could break this patch into smaller, but related, pieces.
If that helps the review process....%%%

%%%Although I *could* break this patch into smaller, but related, pieces. If that helps the review process....%%%

%%%It's not necessary to break up the patch, would not help much. It might take a few days until I get to reviewing and committing it.%%%

%%%It's not necessary to break up the patch, would not help much. It might take a few days until I get to reviewing and committing it.%%%
Author
Member

%%%It would be nice to have this reviewed fairly soon as it is a large patch. I will have time over the next few weeks to make changes if they are necessary, but will have less time starting in January.%%%

%%%It would be nice to have this reviewed fairly soon as it is a large patch. I will have time over the next few weeks to make changes if they are necessary, but will have less time starting in January.%%%

%%%I'll do this after the release, didn't have enough time to do before. Probably won't require further work from you anyway, the way it's implemented seems fine.%%%

%%%I'll do this after the release, didn't have enough time to do before. Probably won't require further work from you anyway, the way it's implemented seems fine.%%%

%%%Hi,
is this implementation really the best one?
I mean, I see the reason for it, this way we only have 1 default variable per property, which can be used in RNA and on the lower level like camera.c etc.

Imho this is polluting DNA files too much. It's not entirely clear to me which default Value belongs to which property, while only looking at the DNA files.

The "Reset to Default" value uses the defaults in RNA, so I'd rather only add the defaults in RNA and keep the rest as is. %%%

%%%Hi, is this implementation really the best one? I mean, I see the reason for it, this way we only have 1 default variable per property, which can be used in RNA and on the lower level like camera.c etc. Imho this is polluting DNA files too much. It's not entirely clear to me which default Value belongs to which property, while only looking at the DNA files. The "Reset to Default" value uses the defaults in RNA, so I'd rather only add the defaults in RNA and keep the rest as is. %%%
Author
Member

%%%Thomas,

That is exactly as I did it originally and you can find that in patches #32862, #32859, and #32854.
Those just used constants like "RNA_def_property_int_default(prop, 44100)".

But then Campbell asked that all those constants be replaced with defines in a single file, and
that is "DefaultsAsDefines.diff"

After that Brecht asked that these instead go into the appropriate DNA file and be used to initialize
there as well. That is the "DefaultsAsDefines2.diff" version.

%%%

%%%Thomas, That is exactly as I did it originally and you can find that in patches #32862, #32859, and #32854. Those just used constants like "RNA_def_property_int_default(prop, 44100)". But then Campbell asked that all those constants be replaced with defines in a single file, and that is "DefaultsAsDefines.diff" After that Brecht asked that these instead go into the appropriate DNA file and be used to initialize there as well. That is the "DefaultsAsDefines2.diff" version. %%%
Author
Member

%%%I've made a smaller patch against the current SVN (54976) to help with testing this out.
"DefaultsAsDefinesScene.diff" adds defaults to just the "scene" tab of the properties editor.
My hope is that you try this to see whether this feature is useful or if we should just remove
the "Reset to Default Values" item.%%%

%%%I've made a smaller patch against the current SVN (54976) to help with testing this out. "DefaultsAsDefinesScene.diff" adds defaults to just the "scene" tab of the properties editor. My hope is that you try this to see whether this feature is useful or if we should just remove the "Reset to Default Values" item.%%%

Added subscribers: @ideasman42, @Sergey

Added subscribers: @ideasman42, @Sergey

Can see the use of such place but implementation could be simplified i think. For example, we don't really need dedicated defines for the defaults only used by RNA. Unless the value is actually expected to be re-used by DNA struct allocation/initialization and RNA i wouldn't bother with dedicated define.

On the other hand, those values are never changing. @ideasman42 has a nice idea, he is typing it now :)

Can see the use of such place but implementation could be simplified i think. For example, we don't really need dedicated defines for the defaults only used by RNA. Unless the value is actually expected to be re-used by DNA struct allocation/initialization and RNA i wouldn't bother with dedicated define. On the other hand, those values are never changing. @ideasman42 has a nice idea, he is typing it now :)

Adding 1000's of defines is also giving us work to manage... (though it does make some sense).

What we could do instead:

  • Add a test file (in tests/python/rna_defaults.py)
  • The script can create new data scene, object... etc
  • The script compare initial values with the RNA defaults and raise errors when they don't match.

This way we have an automated way of keeping the values in sync without adding defines all over.

Adding 1000's of defines is also giving us work to manage... (though it does make some sense). What we could do instead: - Add a test file *(in `tests/python/rna_defaults.py`)* - The script can create new data scene, object... etc - The script compare initial values with the RNA defaults and raise errors when they don't match. This way we have an automated way of keeping the values in sync without adding defines all over.

Made a script to check defaults are OK, (as described above) P219

Made a script to check defaults are OK, (as described above) [P219](https://archive.blender.org/developer/P219.txt)

Changed status from 'Open' to: 'Resolved'

Changed status from 'Open' to: 'Resolved'
Campbell Barton self-assigned this 2015-04-30 17:26:39 +02:00

Resolved 3a9726783f

Resolved 3a9726783f
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
5 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#32894
No description provided.