Fix #119534: Action Editor empty channels region without active action #119621

Philipp Oeser · 2024-03-18T12:11:51+01:00

Philipp Oeser commented

2024-03-18 12:11:51 +01:00

Caused by a96f1208cc

For SPACE_ACTION (SACTCONT_ACTION), actedit_get_context will
return true, but ANIM_animdata_context_getdata also checks the
bAnimContext data [which in this case is the action], so will not
return true.

As a result we are skipping drawing the background and the search, which
is now done again.

Caused by a96f1208cc72 For `SPACE_ACTION` (`SACTCONT_ACTION`), `actedit_get_context` will return true, but `ANIM_animdata_context_getdata` also checks the `bAnimContext` data [which in this case is the action], so will **not** return true. As a result we are skipping drawing the background and the search, which is now done again.

Philipp Oeser added 1 commit 2024-03-18 12:12:01 +01:00

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

cacf82a477 Fix #119534 : Action Editor empty channels region without active action

Caused by a96f1208cc

For `SPACE_ACTION` (`SACTCONT_ACTION`), `actedit_get_context` will
return true, but `ANIM_animdata_context_getdata` also checks the
`bAnimContext` data [which in this case is the action], so will **not**
return true.

As a result we are skipping drawing the background and the search, which
is now done again.

Philipp Oeser added this to the Animation & Rigging project 2024-03-18 12:12:16 +01:00

Philipp Oeser requested review from Christoph Lendenfeld 2024-03-18 12:12:23 +01:00

Philipp Oeser requested review from Sybren A. Stüvel 2024-03-18 12:12:28 +01:00

Philipp Oeser referenced this pull request

2024-03-18 12:12:58 +01:00

Pink channels region in Action Editor #119534

Christoph Lendenfeld approved these changes 2024-03-18 22:34:27 +01:00

Christoph Lendenfeld left a comment

thanks for the fix. I will approve this since the fact that the bAnimContext is usable even though false is returned shouldn't stand in the way of this fix. Do wait for sybren to have eyes on this though

thanks for the fix. I will approve this since the fact that the `bAnimContext` is usable even though false is returned shouldn't stand in the way of this fix. Do wait for sybren to have eyes on this though

source/blender/editors/space_action/space_action.cc

						
				@ -287,0 +289,4 @@

				  UI_ThemeClearColor(TH_BACK);

				  /* channel filter next to scrubbing area */

				  ED_time_scrub_channel_search_draw(C, region, ac.ads);

Christoph Lendenfeld commented

2024-03-18 22:24:02 +01:00

The fact that has_valid_animcontext is false, but you can still use ac.ads is very confusing.

Now that I think about it, it is probably better to move that to below the if(!has_valid_animcontext).
While I don't see a way how ac.ads could be a nullptr it would be a crash if it is.

~~The only difference is that you wouldn't get the search bar which is fine IMO because there is nothing to search for.~~

Edit: Since this has always worked like that (had a look at my commit that broke this) I am fine landing it like that. It hasn't crashed so far so it's probably fine. Also means no visual changes between 4.0 and 4.1

The fact that `has_valid_animcontext` is false, but you can still use `ac.ads` is very confusing. ~~Now that I think about it, it is probably better to move that to below the `if(!has_valid_animcontext)`. While I don't see a way how `ac.ads` could be a `nullptr` it would be a crash if it is.~~ ~~The only difference is that you wouldn't get the search bar which is fine IMO because there is nothing to search for.~~ Edit: Since this has always worked like that (had a look at my commit that broke this) I am fine landing it like that. It hasn't crashed so far so it's probably fine. Also means no visual changes between 4.0 and 4.1

Sybren A. Stüvel approved these changes 2024-03-19 10:18:29 +01:00

Sybren A. Stüvel left a comment

@ChrisLend wrote:
The fact that has_valid_animcontext is false, but you can still use ac.ads is very confusing.

Ooooooh my, yeah that's bad. Nothing to do with this PR, indeed, so for that just a big thanks @lichtwerk !

> @ChrisLend wrote: > The fact that `has_valid_animcontext` is false, but you can still use `ac.ads` is very confusing. Ooooooh my, yeah that's bad. Nothing to do with this PR, indeed, so for that just a big thanks @lichtwerk !

Philipp Oeser commented

2024-03-19 11:12:15 +01:00

@ThomasDinges : fine for you for 4.1 still?