Anim: Discuss edge cases and add regression tests for NLA strip evaluation #109212

Merged

Nate Rupsis merged 1 commits from pamadini/blender:nla-adjacent-strips-evaluate-later into main

2023-11-20 17:52:37 +01:00

Paolo Amadini commented

2023-06-21 20:09:25 +02:00

Contributor

This pull request has a wider discussion of some NLA edge cases and adds regression tests. The core change that was agreed upon has now been rebased on the latest main branch and moved to #113487.

This changes the behavior of adjacent NLA strips for Blender 4.0, as discussed in the 2023-06-08 Animation & Rigging module meeting.

I'd like to write some regression tests before requesting review.

Optionally, the first part of the nlastrips_ctime_get_strip function could be streamlined with a larger refactoring.

This pull request has a wider discussion of some NLA edge cases and adds regression tests. The core change that was agreed upon has now been rebased on the latest main branch and moved to #113487. --- This changes the behavior of adjacent NLA strips for Blender 4.0, as discussed in the [2023-06-08 Animation & Rigging module meeting](https://devtalk.blender.org/t/2023-06-08-animation-rigging-module-meeting/29793#nla-behavior-last-frame-of-clip-8). I'd like to write some regression tests before requesting review. Optionally, the first part of the `nlastrips_ctime_get_strip` function could be streamlined with a larger refactoring.

Iliya Katushenock added the

Animation & Rigging

label 2023-06-21 20:14:30 +02:00

Sybren A. Stüvel added

Animation & Rigging

and removed

Animation & Rigging

labels 2023-06-22 19:18:58 +02:00

Brad Clark commented

2023-06-27 08:15:10 +02:00

Member

First thanks for exploring the NLA :)

Second I don't think having inconsistent behavior if the clips are on the same track or split should happen, if it changes it so that the next clips start frame is more important to show than the first clips last frame, that should be how it is no matter where the clip is..touching or above/below...the next clip is the next clip.

Third, I don't know how this changes how the HOLD and HOLD forward behavior works with the clips on the same track.

If you can fix it so that it is consistent when the clips are on different tracks and can you please test how this impacts/changes how with two clips with Blending together would work.

Looking testing this in other tools when not touching or blending the last frame of the clip is shown, when they touch or blend, the NEXT clips start frame becomes the winner and is shown.... and I wonder if this would help blending between clips work better in Blender.

First thanks for exploring the NLA :) Second I don't think having inconsistent behavior if the clips are on the same track or split should happen, if it changes it so that the next clips start frame is more important to show than the first clips last frame, that should be how it is no matter where the clip is..touching or above/below...the next clip is the next clip. Third, I don't know how this changes how the HOLD and HOLD forward behavior works with the clips on the same track. If you can fix it so that it is consistent when the clips are on different tracks and can you please test how this impacts/changes how with two clips with Blending together would work. Looking testing this in other tools when not touching or blending the last frame of the clip is shown, when they touch or blend, the NEXT clips start frame becomes the winner and is shown.... and I wonder if this would help blending between clips work better in Blender.

Paolo Amadini commented

2023-06-27 14:53:39 +02:00

Author

Contributor

Thank you @BClark! I had a more complete write-up made before I saw your comment, I originally wanted to update the pull request description but I'll just include the write-up in its original form below, since you didn't have a chance to see it before, and I didn't see you comment either.

Let me know if this answers some or all of your questions. The behavior with clips on a different track is not changed in the pull request based on the assumptions I made. My thinking is still that no change is necessary there, but we can discuss further and I don't feel strongly either way, as it covers a non-default use case that takes a few steps to trigger.

Summary

The goal of this task is to identify and implement the desired behavior for non-linear animation (NLA) strips when evaluated at the boundary of their frame range in Blender 4.0 and beyond.

This investigation is motivated by an agreed inconsistency in Blender 3.x and earlier, when two strips are adjacent on the same track. The last frame of the first strip is evaluated, instead of the first frame of the second strip. This is not so much the result of deliberate design, as much as the result of how the current code iterates through strips.

While it is unambiguous that the first frame of a strip should be evaluated when encountered, there are alternatives about what to do when the time is exactly on the last frame of a strip, and no other strip is present at the same time.

Assumptions

Unless otherwise specified, throughout the discussion it is assumed that the first and last frame of an NLA strip are not extrapolated beyond the boundary, despite the default in Blender being to hold the values of the first and last frames. This means that, when the NLA track is evaluated at a time with no strip, the track has no effect on the result.

The problem space of defining frame ranges in non-linear animation tracks is slightly more nuanced than non-linear video editing software, because frame numbers can be fractional. I’d note that in every case I found in practice, the boundaries are integers, but this can’t be relied upon.

When expressing frame ranges in natural language, it is my experience that the first and last frames are assumed to be included. For example:

A walk cycle exercise would include instructions to set up the scene from frame 1001 to 1033, and set keys at regular intervals on the first, middle, and last frame at 1001, 1017, and 1033. The first and last frames would each be played separately to check that they are equal.
In film production, when a shot runs from frame 1001 to 1010, it would result in 10 frames being rendered. The next shot will start at frame 1011, consistently with how ranges are expressed in non-linear video editing.
A four-frame strip of an eye closing would consist of frames 1, 2, 3, and 4, with frame 1 fully open and 4 fully closed. This means 3 new poses in addition to the first one.

However, when game developers deploy non-linear animation, it may be convenient to reason "mathematically" and never evaluate the last frame. It seems to be the case both in Unity’s Timeline and Unreal’s Sequencer that a clip extending from frames 1 to 4 would evaluate at frames 1, 2, 3, and up to 3.999…, but go back to no evaluation at frame 4.

Inside a strip, animation curves are not necessarily continuous. For example, when constant (stepped) tangents are used on keys at frames 3 and 4, the value at frame 3.999… may be different from the value at frame 4.

Current Behavior and Alternatives

At present, Blender evaluates strips on their first and last frames, both included. My opinion is that this makes sense, thinking of the wider target audience that it has compared to just game development. For example, consider this workflow:

Create a new default scene.
Set a key on the cube on frames 1 and 4.
“Push down” the action in the NLA editor.
The strip extends from frame 1 to 4, and the key on frame 4 is evaluated.
Set “Extrapolation” to “Nothing”.
The strip is still evaluated at frame 4, and back to default at frame 5.

It might be confusing if the last frame was always excluded, as a key would be there and visible in most editors, but it would have no effect. This could also lead to workarounds such as extending the last frame of the strip to just a little over the last frame, such as frame 4.00...01. The exact value to choose would be ambiguous.

Using the last key only when "Extrapolation" is not "Nothing" would mean that the strip would effectively “lose” its last frame when the extrapolation is changed. It would require moving the boundary to keep the last pose. On the other hand, it would be consistent with how game engines work, and address differently the edge case on different tracks that is discussed below.

A new extrapolation mode "Nothing, but include the last frame" would require too much micro-management, and more variables would have to be considered when debugging an NLA sequence.

Adjacent Clips on The Same Track (Changed)

When a strip is manually repeated, a new copy or instance is placed adjacent to the previous one. For example, a cycle from 1001 to 1033 would be followed by the same strip running from 1033 to 1065. In this case, regardless of the extrapolation, frame 1033 should be the first frame of the second strip, which is the only change made by this pull request.

This new behavior would be the same as when the strip is extended and set to repeat (loop) internally. The last frame of the underlying action is only evaluated if the last iteration of the loop falls exactly at the end boundary of the strip. This is the same frame that is held afterwards.

As a side note, Unity has a flag to control looping, and when enabled, the last frame would always be excluded from evaluation. Setting "Post-extrapolate" to "Hold" would hold the first frame of the underlying animation at the end, instead of the last frame.

Clips on Different Tracks (Kept as Now)

If the two strips in the example above have "Extrapolation" set to "Nothing" and one of them is moved to a different track, the value at frame 1033 would be combined based on the strip blending modes. With the default of "Replace", the result would be different based on which clip is above the other.

In game engines, always excluding the last frame has the effect that the result at the boundary does not depend on the order of the tracks, or whether the same track is used for both clips. However, the context in which game developers operate may be different, and what may seem mathematically “cleaner” to reason about for a developer may be more confusing for a target audience that is accustomed to considering the key at the last frame to be significant.

Moreover, with "Hold" being the default extrapolation, the result for all frames, not just 1033, is already different when the clips are moved to a different track. Thus, there may be simply not be enough motivation to change the current behavior for a non-default edge case.

Test File

The following test file has an action running from frames 1 to 4, where the X Location of the cube is the same as the frame number, with constant tangents. This action is referenced by different strips in the NLA editor.

With the change in the pull request, the value at frame 4 will be "1", like it is at frame 22, instead of "4", like it is at frame 13.

Thank you @BClark! I had a more complete write-up made before I saw your comment, I originally wanted to update the pull request description but I'll just include the write-up in its original form below, since you didn't have a chance to see it before, and I didn't see you comment either. Let me know if this answers some or all of your questions. The behavior with clips on a different track is not changed in the pull request based on the assumptions I made. My thinking is still that no change is necessary there, but we can discuss further and I don't feel strongly either way, as it covers a non-default use case that takes a few steps to trigger. ### Summary The goal of this task is to identify and implement the desired behavior for non-linear animation (NLA) strips when evaluated at the boundary of their frame range in Blender 4.0 and beyond. This investigation is motivated by an [agreed inconsistency](https://devtalk.blender.org/t/2023-06-08-animation-rigging-module-meeting/29793#nla-behavior-last-frame-of-clip-8) in Blender 3.x and earlier, when two strips are adjacent on the same track. The last frame of the first strip is evaluated, instead of the first frame of the second strip. This is not so much the result of deliberate design, as much as the result of how the [current code](https://projects.blender.org/blender/blender/src/commit/91ff6457bed71289a88fe34d35ae4ccb4451b73c/source/blender/blenkernel/intern/anim_sys.c#L989-L993) iterates through strips. While it is unambiguous that the first frame of a strip should be evaluated when encountered, there are alternatives about what to do when the time is exactly on the last frame of a strip, and no other strip is present at the same time. ### Assumptions Unless otherwise specified, throughout the discussion it is assumed that the first and last frame of an NLA strip are **not extrapolated** beyond the boundary, despite the default in Blender being to hold the values of the first and last frames. This means that, when the NLA track is evaluated at a time with no strip, the track has no effect on the result. The problem space of defining frame ranges in non-linear animation tracks is slightly more nuanced than non-linear video editing software, because **frame numbers can be fractional**. I’d note that in every case I found in practice, the boundaries are integers, but this can’t be relied upon. When expressing frame ranges **in natural language**, it is my experience that the **first and last frames are assumed to be included**. For example: - A walk cycle exercise would include instructions to set up the scene from frame 1001 to 1033, and set keys at regular intervals on the first, middle, and last frame at 1001, 1017, and 1033. The first and last frames would each be played separately to check that they are equal. - In film production, when a shot runs from frame 1001 to 1010, it would result in 10 frames being rendered. The next shot will start at frame 1011, consistently with how ranges are expressed in non-linear video editing. - A four-frame strip of an eye closing would consist of frames 1, 2, 3, and 4, with frame 1 fully open and 4 fully closed. This means 3 new poses in addition to the first one. However, when **game developers** deploy non-linear animation, it may be convenient to reason "mathematically" and never evaluate the last frame. It seems to be the case both in Unity’s Timeline and Unreal’s Sequencer that a clip extending from frames 1 to 4 would evaluate at frames 1, 2, 3, and up to 3.999…, but go back to no evaluation at frame 4. Inside a strip, animation curves are **not necessarily continuous**. For example, when constant (stepped) tangents are used on keys at frames 3 and 4, the value at frame 3.999… may be different from the value at frame 4. ### Current Behavior and Alternatives At present, Blender evaluates strips on their first and last frames, both included. My opinion is that this makes sense, thinking of the wider target audience that it has compared to just game development. For example, consider this workflow: 1. Create a new default scene. 2. Set a key on the cube on frames 1 and 4. 3. “Push down” the action in the NLA editor. _The strip extends from frame 1 to 4, and the key on frame 4 is evaluated._ 4. Set “Extrapolation” to “Nothing”. _The strip is still evaluated at frame 4, and back to default at frame 5._ It might be confusing if the last frame was always excluded, as a key would be there and visible in most editors, but it would have no effect. This could also lead to workarounds such as extending the last frame of the strip to just a little over the last frame, such as frame 4.00...01. The exact value to choose would be ambiguous. Using the last key only when "Extrapolation" is not "Nothing" would mean that the strip would effectively “lose” its last frame when the extrapolation is changed. It would require moving the boundary to keep the last pose. On the other hand, it would be consistent with how game engines work, and address differently the edge case on different tracks that is discussed below. A new extrapolation mode "Nothing, but include the last frame" would require too much micro-management, and more variables would have to be considered when debugging an NLA sequence. ### Adjacent Clips on The Same Track (Changed) When a strip is manually repeated, a new copy or instance is placed adjacent to the previous one. For example, a cycle from 1001 to 1033 would be followed by the same strip running from 1033 to 1065. In this case, regardless of the extrapolation, frame 1033 should be the first frame of the second strip, which is the only change made by this pull request. This new behavior would be the same as when the strip is extended and set to repeat (loop) internally. The last frame of the underlying action is only evaluated if the last iteration of the loop falls exactly at the end boundary of the strip. This is the same frame that is held afterwards. As a side note, Unity has a flag to control looping, and when enabled, the last frame would always be excluded from evaluation. Setting "Post-extrapolate" to "Hold" would hold the first frame of the underlying animation at the end, instead of the last frame. ### Clips on Different Tracks (Kept as Now) If the two strips in the example above have "Extrapolation" set to "Nothing" and one of them is moved to a different track, the value at frame 1033 would be combined based on the strip blending modes. With the default of "Replace", the result would be different based on which clip is above the other. In game engines, always excluding the last frame has the effect that the result at the boundary does not depend on the order of the tracks, or whether the same track is used for both clips. However, the context in which game developers operate may be different, and what may seem mathematically “cleaner” to reason about for a developer may be more confusing for a target audience that is accustomed to considering the key at the last frame to be significant. Moreover, with "Hold" being the default extrapolation, the result for all frames, not just 1033, is already different when the clips are moved to a different track. Thus, there may be simply not be enough motivation to change the current behavior for a non-default edge case. ### Test File The following test file has an action running from frames 1 to 4, where the X Location of the cube is the same as the frame number, with constant tangents. This action is referenced by different strips in the NLA editor. With the change in the pull request, the value at frame 4 will be "1", like it is at frame 22, instead of "4", like it is at frame 13.

test-nlastrip-boundary.blend

880 KiB

test-nlastrip-boundary.png

42 KiB

Paolo Amadini force-pushed nla-adjacent-strips-evaluate-later from 075cdbc1cb to c60c9acfc0

2023-06-29 17:51:18 +02:00

Compare

Brad Clark commented

2023-06-30 18:54:46 +02:00

Member

I still don't have a good sense for how this is going to feel/change but I hope I can look at it when I have some more rest next week.
Thank you for your interest and looking at the NLA code :)

I have 15 or so issues in my document that are still open and some are very tiny and some are whole new features involving the NLA.

I still don't have a good sense for how this is going to feel/change but I hope I can look at it when I have some more rest next week. Thank you for your interest and looking at the NLA code :) I have 15 or so issues in my document that are still open and some are very tiny and some are whole new features involving the NLA.

Sybren A. Stüvel commented

2023-07-07 12:05:06 +02:00

Member

I really like that you've added new tests for the NLA. How would you feel about moving these into a separate PR, so that they cover the current behaviour of Blender? That way this PR can remain focused on the change in behaviour, keeping the two goals nicely separated. Approving & landing the new tests will be considerably easier than discussing the change in behaviour, so let's keep the latter as small as possible.

I really like that you've added new tests for the NLA. How would you feel about moving these into a separate PR, so that they cover the current behaviour of Blender? That way this PR can remain focused on the change in behaviour, keeping the two goals nicely separated. Approving & landing the new tests will be considerably easier than discussing the change in behaviour, so let's keep the latter as small as possible.

Paolo Amadini commented

2023-07-13 17:47:47 +02:00

Author

Contributor

Thank you, Sybren, for taking a look!

My availability will be limited for the time being, and between the two goals I'd rather focus on making the behavior consistent. However, the tests are already nicely separated in their own commit, and even if they have not landed yet, the core team is very welcome to take the commit and use the code as a template to cover the current behavior, or use it for testing other parts of the nonlinear animation editor.

Thank you, Sybren, for taking a look! My availability will be limited for the time being, and between the two goals I'd rather focus on making the behavior consistent. However, the tests are already nicely separated in [their own commit](https://projects.blender.org/blender/blender/commit/075cdbc1cbe47c9429b85b1f5d99bc0af5bffb63), and even if they have not landed yet, the core team is very welcome to take the commit and use the code as a template to cover the current behavior, or use it for testing other parts of the nonlinear animation editor.

Sybren A. Stüvel commented

2023-07-14 12:47:28 +02:00

Member

Having separate commits is good. The thing is mostly that the test now sits after the change in functionality and thus will need adjustment if they're to be committed to main first.

Having separate commits is good. The thing is mostly that the test now sits *after* the change in functionality and thus will need adjustment if they're to be committed to `main` first.

Paolo Amadini changed title from ~~WIP: Animation: At the boundary frame between non-muted NLA strips, evaluate the first frame of the later strip instead of the last frame of the earlier.~~ to WIP: Discuss edge cases and add regression tests for NLA strip evaluation

2023-10-10 13:15:24 +02:00

Paolo Amadini commented

2023-10-10 13:36:56 +02:00

Author

Contributor

Having separate commits is good. The thing is mostly that the test now sits after the change in functionality and thus will need adjustment if they're to be committed to main first.

Exactly, I was suggesting the team could use the code "as a template" if they wanted to make the adjustments you mention, or if they wanted to add tests to other pull requests that they were working on. This didn't happen in the meantime, since other pull requests touching the same file landed without tests, which is fine as the current convention for that file is that tests are not required.

In fact, I believe you are fundamentally correct, in that adding regression tests and multi-track behavior to the discussion in the same pull request has caused some scope bleed. Thus, I moved the fundamental change to #113487 to keep it separate and not overload reviewers with too much information. I may pick the tests up again and ask for review sometime later, but for now I'd like to save reviewer bandwidth for the change that actually matters the most to the users.

> Having separate commits is good. The thing is mostly that the test now sits *after* the change in functionality and thus will need adjustment if they're to be committed to `main` first. Exactly, I was suggesting the team could use the code "as a template" if they wanted to make the adjustments you mention, or if they wanted to add tests to other pull requests that they were working on. This didn't happen in the meantime, since other pull requests touching the same file landed without tests, which is fine as the current convention for that file is that tests are not required. In fact, I believe you are fundamentally correct, in that adding regression tests and multi-track behavior to the discussion in the same pull request has caused some scope bleed. Thus, I moved the fundamental change to #113487 to keep it separate and not overload reviewers with too much information. I may pick the tests up again and ask for review sometime later, but for now I'd like to save reviewer bandwidth for the change that actually matters the most to the users.

Sybren A. Stüvel commented

2023-10-16 11:45:31 +02:00

Member

This is actually the opposite of what I intended. One of the big issues with the NLA, which makes working on it so cumbersome and risky, is the lack of unit tests. I wasn't trying to point out scope creep, but rather trying to motivate you to take the tests that you have in this PR, and adjust them so that they work on current main. Then a separate PR can alter Blender's behaviour, which would impact both the main code and the tests (which is a good thing).

This is actually the opposite of what I intended. One of the big issues with the NLA, which makes working on it so cumbersome and risky, is the lack of unit tests. I wasn't trying to point out scope creep, but rather trying to motivate you to take the tests that you have in this PR, and adjust them so that they work on current `main`. Then a separate PR can alter Blender's behaviour, which would impact both the main code and the tests (which is a good thing).

Paolo Amadini force-pushed nla-adjacent-strips-evaluate-later from c60c9acfc0 to 5eda2f8766

2023-10-17 14:39:03 +02:00

Compare

Paolo Amadini changed title from ~~WIP: Discuss edge cases and add regression tests for NLA strip evaluation~~ to Discuss edge cases and add regression tests for NLA strip evaluation

2023-10-17 14:40:13 +02:00

Paolo Amadini commented

2023-10-17 14:42:55 +02:00

Author

Contributor

I've taken the tests and adjusted them so that they work on current main. Who should I ask for review?

I've taken the tests and adjusted them so that they work on current `main`. Who should I ask for review?

Sybren A. Stüvel commented

2023-10-23 10:37:56 +02:00

Member

You can add @nrupsis and me as reviewers.

You can add @nrupsis and me as reviewers.

Paolo Amadini requested review from Sybren A. Stüvel 2023-10-24 17:18:22 +02:00

Paolo Amadini requested review from Nate Rupsis 2023-10-24 17:18:22 +02:00

Nate Rupsis reviewed 2023-10-25 20:30:40 +02:00

tests/python/bl_animation_nla_strip.py Outdated

						
				@ -0,0 +1,120 @@

				# SPDX-FileCopyrightText: 2023 Blender Authors

				#

				# SPDX-License-Identifier: GPL-2.0-or-later

Nate Rupsis commented

2023-10-24 21:53:36 +02:00

Member

Just double checking we're using GPL-2.0

I'm seeing a lot of test files with SPDX-License-Identifier: Apache-2.0

Just double checking we're using GPL-2.0 I'm seeing a lot of test files with `SPDX-License-Identifier: Apache-2.0`

Paolo Amadini commented

2023-10-29 17:38:27 +01:00

Author

Contributor

Both seem to be in use, happy to switch if it's needed once you've determined which license works best for new files here.

Both seem to be in use, happy to switch if it's needed once you've determined which license works best for new files here.

Sybren A. Stüvel commented

2023-11-02 16:48:25 +01:00

Member

Blender is "GPLv2.0 or later". Other code could have been submitted with a different (but compatible) license. When in doubt, use GPLv2+.

Blender is "GPLv2.0 or later". Other code could have been submitted with a different (but compatible) license. When in doubt, use GPLv2+.

dr.sybren marked this conversation as resolved

tests/python/bl_animation_nla_strip.py Outdated

						
				@ -0,0 +27,4 @@

				    def setUpClass(cls):

				        bpy.ops.wm.read_factory_settings()

				        cls.test_object = bpy.data.objects["Cube"]

Nate Rupsis commented

2023-10-25 20:27:00 +02:00

Member

Might be worth creating / tearing down an object as well, that way we're not reliant on the default Blender scene which could (probably won't) change.

Might be worth creating / tearing down an object as well, that way we're not reliant on the default Blender scene which could (probably won't) change.

pamadini marked this conversation as resolved

tests/python/bl_animation_nla_strip.py Outdated

						
				@ -0,0 +47,4 @@

				        strip.extrapolation = "NOTHING"

				        return strip

				    def assertFrameValue(self, frame: float, value: float):

Nate Rupsis commented

2023-10-25 20:29:31 +02:00

Member

Would change the parameter name to "expected_value". Gives a clearer indication what the method does.

Would change the parameter name to "expected_value". Gives a clearer indication what the method does.

pamadini marked this conversation as resolved

tests/python/bl_animation_nla_strip.py Outdated

						
				@ -0,0 +117,4 @@

				if __name__ == "__main__":

				    # Drop all arguments before "--", or everything if the delimiter is absent. Keep the executable path.

				    unittest.main(argv=sys.argv[:1] + (sys.argv[sys.argv.index("--") + 1:] if "--" in sys.argv else []))

Nate Rupsis commented

2023-10-24 21:51:40 +02:00

Member

(Unrelated to this PR. ) This seems like a pretty consistent pattern across the python tests.

@dr.sybren would it make sense for this to be abstracted into a Util of some sort?

(Unrelated to this PR. ) This seems like a pretty consistent pattern across the python tests. @dr.sybren would it make sense for this to be abstracted into a Util of some sort?

Sybren A. Stüvel commented

2023-11-02 16:47:32 +01:00

Member

Maybe at some point, but not in this PR ;-)

Maybe at some point, but not in this PR ;-)

dr.sybren marked this conversation as resolved

Paolo Amadini force-pushed nla-adjacent-strips-evaluate-later from 5eda2f8766 to 8db7f4f041

2023-10-29 17:34:43 +01:00

Compare

Nate Rupsis approved these changes 2023-11-02 16:56:04 +01:00

Nate Rupsis left a comment

Member

Awesome to see some python tests for the NLA!

Awesome to see some python tests for the NLA!

Sybren A. Stüvel commented

2023-11-02 17:52:42 +01:00

Member

@pamadini for some reason anonymous access to https://projects.blender.org/pamadini/blender is blocked. Was this something you configured? To integrate well with other systems, please make sure the project is publicly accessible.

@pamadini for some reason anonymous access to https://projects.blender.org/pamadini/blender is blocked. Was this something you configured? To integrate well with other systems, please make sure the project is publicly accessible.

Nate Rupsis commented

2023-11-02 17:55:40 +01:00

Member

@pamadini for some reason anonymous access to https://projects.blender.org/pamadini/blender is blocked. Was this something you configured? To integrate well with other systems, please make sure the project is publicly accessible.

Huh, his fork is loading fine for me...

> @pamadini for some reason anonymous access to https://projects.blender.org/pamadini/blender is blocked. Was this something you configured? To integrate well with other systems, please make sure the project is publicly accessible. > Huh, his fork is loading fine for me...

Sybren A. Stüvel commented

2023-11-03 14:24:03 +01:00

Member

It loads fine when I'm logged in. When in a private window, though, I get a "404 not found" error.

It loads fine when I'm logged in. When in a private window, though, I get a "404 not found" error.

Nate Rupsis commented

2023-11-03 15:32:07 +01:00

Member

It loads fine when I'm logged in. When in a private window, though, I get a "404 not found" error.

Oh okay, I'm experiencing that same behavior.

> It loads fine when I'm logged in. When in a private window, though, I get a "404 not found" error. Oh okay, I'm experiencing that same behavior.

Paolo Amadini commented

2023-11-07 20:43:46 +01:00

Author

Contributor

It's fixed now! I noticed the same behavior, I thought it was expected but it turns out I've just flipped a profile setting at some point.

It's fixed now! I noticed the same behavior, I thought it was expected but it turns out I've just flipped a profile setting at some point.

Paolo Amadini force-pushed nla-adjacent-strips-evaluate-later from 8db7f4f041 to 6eea97b236

2023-11-07 20:54:14 +01:00

Compare

Paolo Amadini requested review from Nate Rupsis 2023-11-07 20:56:06 +01:00

Paolo Amadini commented

2023-11-07 20:58:33 +01:00

Author

Contributor

I rebased this on the latest main and I've re-requested review to be able to land the updated version.

I rebased this on the latest `main` and I've re-requested review to be able to land the updated version.

Nate Rupsis approved these changes 2023-11-10 20:43:09 +01:00

Nate Rupsis left a comment

Member

Make sure to prefix the PR title with "Anim:"

https://wiki.blender.org/wiki/Style_Guide/Commit_Messages

Make sure to prefix the PR title with "Anim:" https://wiki.blender.org/wiki/Style_Guide/Commit_Messages

Paolo Amadini changed title from ~~Discuss edge cases and add regression tests for NLA strip evaluation~~ to Anim: Discuss edge cases and add regression tests for NLA strip evaluation

2023-11-12 14:37:00 +01:00

Paolo Amadini commented

2023-11-12 14:56:58 +01:00

Author

Contributor

Make sure to prefix the PR title with "Anim:"

Done, and the commit message already started with "Animation".

The documentation seems to suggest that the final merge should be done by the reviewer? I still don't seem to have permission, even though I now have an up-to-date review and I've rebased the code.

> Make sure to prefix the PR title with "Anim:" Done, and the commit message already started with "Animation". The [documentation](https://wiki.blender.org/wiki/Tools/Pull_Requests#Merge_a_Pull_Request) seems to suggest that the final merge should be done by the reviewer? I still don't seem to have permission, even though I now have an up-to-date review and I've rebased the code.

Nate Rupsis commented

2023-11-13 05:15:42 +01:00

Member

Make sure to prefix the PR title with "Anim:"

Done, and the commit message already started with "Animation".

The documentation seems to suggest that the final merge should be done by the reviewer? I still don't seem to have permission, even though I now have an up-to-date review and I've rebased the code.

About 2 months back we decided on the "Anim" prefix. probably should document it somewhere 😬

I can merge this in if you want :)

> > Make sure to prefix the PR title with "Anim:" > > Done, and the commit message already started with "Animation". > > The [documentation](https://wiki.blender.org/wiki/Tools/Pull_Requests#Merge_a_Pull_Request) seems to suggest that the final merge should be done by the reviewer? I still don't seem to have permission, even though I now have an up-to-date review and I've rebased the code. About 2 months back we decided on the "Anim" prefix. _probably_ should document it somewhere 😬 I can merge this in if you want :)

Paolo Amadini force-pushed nla-adjacent-strips-evaluate-later from 6eea97b236 to e4b2a428a0

2023-11-19 18:21:50 +01:00

Compare

Paolo Amadini commented

2023-11-19 18:25:56 +01:00

Author

Contributor

About 2 months back we decided on the "Anim" prefix

Fixed!

I can merge this in if you want :)

Yes, thanks, I don't seem to have the required permissions to perform the merge after the review.

> About 2 months back we decided on the "Anim" prefix Fixed! > I can merge this in if you want :) Yes, thanks, I don't seem to have the required permissions to perform the merge after the review.

Nate Rupsis merged commit da29519c7a into main

2023-11-20 17:52:37 +01:00

Nate Rupsis referenced this issue from a commit

2023-11-20 17:52:38 +01:00

Anim: Add regression tests for NLA strip evaluation

Sybren A. Stüvel commented

2023-12-05 11:13:39 +01:00

Member

Yes, thanks, I don't seem to have the required permissions to perform the merge after the review.

For the record: this is how Gitea works. You either have commit access (and can commit whatever), or you don't. Having an accepted PR doesn't give you the right to land that PR, so unless you're part of the regular dev team, it'll be the reviewer who lands the PR. This is fine, you'll still be registered as author of the commit.

> Yes, thanks, I don't seem to have the required permissions to perform the merge after the review. For the record: this is how Gitea works. You either have commit access (and can commit whatever), or you don't. Having an accepted PR doesn't give you the right to land that PR, so unless you're part of the regular dev team, it'll be the reviewer who lands the PR. This is fine, you'll still be registered as author of the commit.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 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#109212

No description provided.