fix: Bevel highlight direction back to v1.0.2 algorithm; keep old files rendering unchanged by 036006 · Pull Request #3784 · synfig/synfig

036006 · 2026-07-24T05:38:52Z

The Bevel layer renders its highlight (Hi-Color) rotated ~30° CW compared to Synfig 1.0.2 — and has done so since ~v1.3.11 (bisect in #3769). This PR restores the original 1.0.2-era algorithm as Bevel v0.4, fixes two genuinely Cobra-era indexing defects, and keeps old project files rendering exactly as before.

Part 1. Bevel algorithm fixes (bevel.cpp → v0.4)

1. Highlight direction rotated ~30° CW (the main issue, #3769) — long-standing, not Cobra-era

Symptom: the highlight is rotated ~27–37° CW compared to Synfig 1.0.2, in old-renderer and Cobra builds alike.
Mechanism: the 6-tap gradient estimate takes central differences along d0 (angle a), d1 (a−45°) and d2 (perpendicular). As written, the two d2 taps make the sampling fan asymmetric — {θ−135°, θ−45°, θ} instead of {θ−45°, θ, θ+45°} — skewing the estimated bevel axis by ~30°.
Fix: swapped +=/-= on the two d2 lines, restoring the symmetric fan. Verified by the render matrix above: v0.4 matches 1.0.2.
Note: the d2 lines are textually unchanged since 1.0.2, so the regression's exact origin between 1.3.10 and 1.3.11 is not in the layer itself (no semantic changes in that window in bevel.cpp, blur.cpp, or renddesc). The render matrix is the ground truth this fix is validated against. (Commit message "correct highlight direction after Cobra port" misattributes the origin — the port only preserved the pre-existing behavior.)

2. Wrong indexing with target_min (Cobra-era)

Symptom: the bevel drifts or distorts in tiled rendering and for layers with shifted bounds.
Cause: TaskBevelSW::run started its loop at u = halfsizex + abs(offset_u) + target_min[0] (same for v). But blurred is indexed from zero relative to the expanded work surface, while target_min is the absolute origin of the target rectangle. The correct start at ix = target_min[0] is exactly halfsizex + abs(offset_u) — without target_min. The bug is invisible when target_rect.min == 0 (single full-canvas render), which is why it slipped through.
Fix: removed + target_min[0] / + target_min[1] from the u/v initialization.

3. Half-size mismatch for GAUSSIAN blur (Cobra-era)

Symptom: with Blur::GAUSSIAN the bevel was computed with wrong geometry.
Cause: set_coords_sub_tasks sized the buffer with the GAUSSIAN_ADJUSTMENT formula, but run() indexed it with the generic fabs(size*.5/pw)+3 formula. The half-size logic was duplicated in two places and drifted apart. (The default FASTGAUSSIAN type was unaffected — the formulas match there.)
Fix: extracted one helper bevel_calc_halfsize() used both to size the buffer and to index it. No duplication left, so they cannot drift again.

Part 2. Backward compatibility (loadcanvas.cpp + bevel_deprecated)

The 1.3.x–1.5.5 rendering is what existing artwork actually looks like, so it is preserved as a new layer type bevel_deprecated (an unchanged copy of the v0.3 layer); Bevel itself is bumped to v0.4 with the 1.0.2-era algorithm.
Migration on load: CanvasParser::parse_layer normalizes the layer type in one block (following the filled_rectangle → rectangle pattern). A bevel of version 0.2–0.3 on canvas ≥ 1.1 is loaded as bevel_deprecated, preserving its old render result; otherwise it loads as the new bevel. This implements the plan from Bevel Layer renders incorrectly comparing to version 1.0.2 #3769: canvas ≥ 1.1 ⟺ Synfig ≥ 1.3.12, i.e. the era with the rotated highlight.
bevel_deprecated is hidden from the New Layer menu (CATEGORY_DO_NOT_USE, same as svg_layer) and from the CLI layer list, while remaining instantiable by name for loading and for the Upgrade action.
A warning is shown, mentioning that the layer can be updated via "Upgrade Layer" in the layer context menu.

Part 3. New "Upgrade Layer" action (synfig-studio)

A generic context-menu action (task upgrade), driven by a data map deprecated → current (currently bevel_deprecated → bevel). Adding a new pair is one line in get_target_layer_name().
Behavior: appears in the layer context menus when all selected layers are in the map. Carries over parameters (set_param_list — the vocab is identical), description (user-set only; a default one falls back to the new layer's name), active state, exclude_from_rendering, group membership (Canvas::insert syncs group_db_), and animated parameters (ValueNodes are shared, not cloned, so exported/animated links stay referenced). Full undo/redo via the action super-class.
Multi-select safety: layers are processed in descending depth order so index shifts never affect pending conversions.

Part 4. Icons

New action_layer_upgrade_icon (.sif asset) wired into known_icon_list (iconcontroller.cpp), so the upgrade task gets its stock icon; added to images/CMakeLists.txt and Makefile.am for the .sif → .png render.
The bevel_deprecated layer reuses the old bevel icon with an exclamation mark added, to visually mark it as deprecated.

…alculations

…fixes GAUSSIAN half-size mismatch

…yer), map bevel_deprecated->bevel

…onversion

gemini-code-assist · 2026-07-24T05:38:55Z

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

codacy-production · 2026-07-24T05:40:53Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 79 complexity · 43 duplication

Metric Results

Complexity 79

Duplication 43

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

rodolforg

Thank you for fixing this!
(I didn't understand the 'indexing' word in the PR title.)

rodolforg · 2026-07-24T14:48:26Z

+	halfsizex = (int)(std::fabs(size_x*.5/pw) + 3);
+	halfsizey = (int)(std::fabs(size_y*.5/ph) + 3);
+


These lines can be moved to inside the first (agglomerated) branch of the switch statement below.
These values are not used for Blur::GAUSSIAN

Done, thanks. The generic formula now lives only in the agglomerated branch.
I also added a default: label to that branch.

rodolforg · 2026-07-24T14:54:14Z

+src/modules/lyr_std/bevel_deprecated.cpp
+src/modules/lyr_std/bevel_deprecated.h


I don't think we need a new layer for solve this compatibility issue.
We could just use the layer (bad) version to use the TaskBevelDeprecated in Layer_Bevel::build_composite_fork_task_vfunc.

The new layer was introduced, because the following reason:
If you load synfig file with old (broken) version, then Synfig will bump layer version.
So, when you load the file after saving it will behave as new (fixed) version, which might not be desirable for user.

As solution, when broken version id loaded we are converting it to Deprecated Bevel. This allows to keep compatibility.

What about adding a new (invisible) parameter as I did for that broken gradient stuff?

I went with a separate layer type for two reasons:

It quarantines the legacy code in one file pair that a future release
can drop wholesale, without touching bevel.cpp. A hidden version flag
leaves a legacy branch inside the current layer forever.

Users get an explicit, undoable way to migrate old files to the fixed
algorithm (Upgrade Layer in the layer menu). A hidden flag has no UI
affordance — the only way to "upgrade" would be recreating the layer or
hand-editing.

1. It quarantines the legacy code in one file pair that a future release can drop wholesale, without touching bevel.cpp. A hidden version flag leaves a legacy branch inside the current layer forever.

I don't think we could 'ever' remove the legacy code. We could have ourselves old animations that we don't touch for years and then, when we want to do so, Synfig would render it wrong - as MS Word does with its documents between versions :(

With the hidden flag would be not as simple, but it would be a search for "if (flag)" and wouldn't need to remove the deprecated filenames off the CMake/Autotools/POTFILE.in and include headers ;)

2. Users get an explicit, undoable way to migrate old files to the fixed algorithm (Upgrade Layer in the layer menu). A hidden flag has no UI affordance — the only way to "upgrade" would be recreating the layer or hand-editing.

Your great idea and work can ... work... even with the hidden flag too. Wouldn't be a simple change in get_target_layer_name()? It would have a layer name and its flag of deprecation instead?
Maybe this info could even be in a 'database' in Layer class. So loadcanvas and this new synfigapp action would be always in sync.

rodolforg · 2026-07-24T15:04:10Z

+namespace lyr_std
+{
+
+class Layer_BevelDeprecated : public Layer_CompositeFork


If we want to keep this other version, maybe it should disappear of the Add Layer menu?
If so,

make it inherit the Layer_Invisible class too.

remove from the lyr_std/main.cpp

include this header directly in loadcanvas.cpp (as Layer_Mime is, as it shouldn't appear to final user)
And take a look about the strategy I used in fix: inverted interpolation of gradient since 1.3.11 #3477 and see it fits.

If we want to keep this other version, maybe it should disappear of the Add Layer menu?
Yes, it should disappear.

Done — hidden via CATEGORY_DO_NOT_USE, the same mechanism svg_layer uses (mod_svg/layer_svg.cpp:58).

I kept the registration and used the category instead: unregistered,
Layer::create("bevel_deprecated") would fall back to Layer_Mime and break
the migration and the Upgrade action. Layer_Invisible wouldn't help
either — stroboscope, timeloop and freetime inherit it and are still in
the menu; the filter is category-based.

rodolforg · 2026-07-24T15:18:16Z

+private:
+	std::list<synfig::Layer::Handle> layers;
+
+	void prepare_upgrade_layer(const synfig::Layer::Handle &layer, const synfig::String &target_name);


please prefer the code style that places the reference character & next to type, not near the variable name (C-style).

Done, thanks — all references in layerupgrade now use Type& style.

…ch branch

036006 · 2026-07-28T06:30:25Z

Pushed a round of review fixes:

half-size helper: generic formula moved into the agglomerated switch
branch (+ a default case to keep defined behavior for invalid types);
bevel_deprecated hidden from the New Layer menu / CLI list via
CATEGORY_DO_NOT_USE;
code style: reference placement in layerupgrade.

Also corrected the origin story in the description: a render matrix against
the #3769 test file shows the highlight rotation predates the Cobra port
(present since ~1.3.11); the port preserved it faithfully, and v0.4 matches
the 1.0.2 algorithm.

morevnaproject · 2026-08-27T15:53:30Z

I think we can merge this.

rodolforg · 2026-08-30T15:02:09Z

I think we can merge this.

Please see this comment: #3784 (comment)

rodolforg · 2026-09-03T10:08:30Z

@036006 @morevnaproject That's what I meant:
138da74

036006 added 9 commits July 24, 2026 10:58

add bevel_deprecated layer as a copy of bevel

5845890

fix(bevel): correct highlight direction after Cobra port

8941c29

fix(bevel): remove unnecessary addition of target_min in coordinate c…

9919462

…alculations

fix(bevel): refactor half-size calculation into a separate function; …

b74ae8f

…fixes GAUSSIAN half-size mismatch

chore(bevel): bump version to 0.4

9c4c79d

feat(loadcanvas): migrate legacy bevel to bevel_deprecated on load

d3a5d2d

feat(studio): add generic LayerUpgrade action (deprecated->current la…

05ed188

…yer), map bevel_deprecated->bevel

fix(loadcanvas): enhance warning message for deprecated bevel layer c…

8880ef2

…onversion

fix(iconcontroller): align spacing in layer icon names for consistency

8936e18

036006 changed the title ~~Fix bevel angle~~ Fix Bevel highlight direction and indexing after Cobra port Jul 24, 2026

fix(po): add bevel_deprecated and layerupgrade sources to POTFILES.in

a8263c6

036006 changed the title ~~Fix Bevel highlight direction and indexing after Cobra port~~ fix: Bevel highlight direction and indexing after Cobra port Jul 24, 2026

rodolforg requested changes Jul 24, 2026

View reviewed changes

rodolforg reviewed Jul 24, 2026

View reviewed changes

036006 added 3 commits July 28, 2026 12:45

style(layerupgrade): place reference & next to type

07d0e26

fix(bevel): move generic half-size formula into the agglomerated swit…

613f243

…ch branch

fix(bevel_deprecated): hide deprecated layer from the New Layer menu

e19026c

036006 changed the title ~~fix: Bevel highlight direction and indexing after Cobra port~~ fix: Bevel highlight direction back to v1.0.2 algorithm; keep old files rendering unchanged Jul 28, 2026

		halfsizex = (int)(std::fabs(size_x*.5/pw) + 3);
		halfsizey = (int)(std::fabs(size_y*.5/ph) + 3);

		src/modules/lyr_std/bevel_deprecated.cpp
		src/modules/lyr_std/bevel_deprecated.h

Uh oh!

Conversation

036006 commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Part 1. Bevel algorithm fixes (bevel.cpp → v0.4)

Part 2. Backward compatibility (loadcanvas.cpp + bevel_deprecated)

Part 3. New "Upgrade Layer" action (synfig-studio)

Part 4. Icons

Uh oh!

gemini-code-assist Bot commented Jul 24, 2026

Uh oh!

codacy-production Bot commented Jul 24, 2026

Up to standards ✅

Uh oh!

rodolforg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

036006 commented Jul 28, 2026

Uh oh!

morevnaproject commented Aug 27, 2026

Uh oh!

rodolforg commented Aug 30, 2026

Uh oh!

rodolforg commented Sep 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

036006 commented Jul 24, 2026 •

edited

Loading