- User Since
- Nov 28 2009, 10:22 PM (568 w, 3 d)
I think proper approach here would be to ensure there is a solid workflow is laid down. The issue with what is being discussed here is not only comes down to the lack of preview, but also to a rather cumbersome interaction with entities.
Do we have an automated regression tests for sequencer?
I think those warnings are nice to be officially enabled one day, so please add them to the list in T78535.
Please grab latest Linux build from builder.blender.org and see if it works.
Currently non-fully-formalized rule of thumb is to do ensure_evaluated at the top level, and pass evaluated depsgraph. The function must be clear whether it expects things to be evaluated or not. The reason for this is to avoid re-evaluations o nthe "bottom" levels when, say, operator does modify a lot of objects. So general flow is such:
Mon, Oct 19
Tagging and forcing an update for something what is needed 5 levels deeper in the callstack does not sound correct at all.
This part seems fine. Please commit it.
First of all, the relations builder should be doing same tag as in nodes builder:
From quick read seems fine.
In the future it would need to be split further, to separate rendering from cache and things like that. But baby steps!
Please make sure your IDE is configured for the clang-format.
It might be re-introduced issue, might be different codepath.
It seems the move construct must use noexcept
The "bump" comment witnesses the popularity of this request
Fri, Oct 16
There is no update. Team is overloaded with a lot of other projects. It is still a planned-to-be-worked-on project, but there are no time allocated for it a far as I know. As soon as there is anything, you'll see it reflected in this task.
There is a convention, there is a guide which is worded in terms of "preferred". If we don't change the wording/meaning/convention I would like not to see project-wide cleanups (which switches from acceptable to preferred state).
Much excitement is going on here now! There is one point I am forgetting to raise about
In my opinion, we shouldn't even be committing commented-out code.
- ✓ Allows to more visually separate English from code
- ✗ Verbose
- ✗ Requires trade-off for */ on the same line vs. next line
- ✗ Unavoidable in C++ code (violating use-for-English-only rule)
The patch has been actually committed in rB9f48a04799f. The D number got lost, so the review didn't close automatically.
@Ivan Perevala (ivpe), if there are no obvious flaws I'd like to move on with this change and get it in master :)
There are some points we agree on indeed.
Thu, Oct 15
That is a good find!
Code is fine. The commit message can use some simplification in terms of sentence length. My take in the simplification:
Suggestion is to unify automask vs. automasking in variable naming. For the rest seems rather straightforward change. Testing I'll leave up to you :)
From the code side seems fine. See the inlined comment, that part seems to be related to an independent fix related on correct order of bounding box corners.
Not sure if others want to have a review here.
That is a very good question :)
Configure dynamic paint with subframes would be one of the ways to hit the codepath.
After spending some time trying to make it less fragile solution, it seems to be most reliable to remove ID_RECALC_ALL. It is only couple of places where it requiers some brain cells, the rest seems to be straightforward. I'll start moving towards that direction.
The code as it is now requires two places where "serialization-before-loop-is-finished" happens. This is intrinsicly proone to errors. Is it possible to handle both max-distance-reached and no-vertices-to-be-processed in the single statement? Something like
Wed, Oct 14
-8392829 is an weird value. It might be coming from ID_RECALC_ALL and some bits cleared explicitly.
Can you try to replace ID_RECALC_ALL = ~(0) with ID_RECALC_ALL = ((1 << 25) - 1) ?
Seems reasonable. FFmpeg source was always source of truth for such things anyway.
@Peter Fog (tintwotin), such things are heavily GPU+driver specific. Can't say for everyone, but doesn't happen on my laptop, but its Linux and older GPU.
Added D9211 which sanitizes the keying node alpha behaviour.
Is my understanding correct, that the error happens because cast attempts to convert int-value to IDRecalcFlag and the value does not exist in any of the enumerator items?
Is it signess issue? Will 1u << bitscan_forward_clear_i(...) + makign current_flag unsigned resolve the error?
There is still 1 week in bcon2 for new features, and then about 3-4 weeks in bcon3 for bugfix.
Is implementation of bending springs really a 3 months project? What's the status of it?
Fix typo in comment.
The code seems fine.
The description of the change can explicitly mention the solution: scale the fade with an empirically found scale factor.
Tue, Oct 13
From code side seems fine.
From presentation level -- would be more insightful to have side-by-side comparison of old/new behavior.
This patch seems fine. Isolated and does one thing only.
Keying node violates pre-multiplication of the compositing buffer: it sets alpha without multiplying color channels. The original motivation behind this was to be able to chain multiple keying nodes. But now it seems that this decision causes more problems than solutions, so probably need to ensure pre-multiplication of result at where it is expected to be.
@Sybren A. Stüvel (sybren), I don't think it is the motion paths themselves to be looked into, but more like a system integration type of a thing. One thing to keep in mind is that calculation of motion path could be very resource-involved (both CPU time and memory).
Mon, Oct 12
Thanks for the time spent in investigation and working on the patch. However, I don't think this is a good way to go. There are few aspects: