android / enable scalarBlockLayout

[billzez]

android / enable scalarBlockLayout

[jpbruyere]

I've merge both fixes on master this morning, and remote git https://github.com/jpbruyere/vkvg/tree/master/src/recording/vkvg_record_internal.h looks good to me, try maybe with a clean git clone, sometimes with rebase there are some changes in remote history that are not applied locally. Wait, that's recording=false, I check...ok you're right, I make the fix now. Done, the fix is pushed on master, thanks for the report.

[billzez]

One more comment. vkGetPhysicalDeviceFeatures2 is a 1.1 feature, maybe update the readme that 1.1 is a minumum requirement.

[billzez]

or just:

#ifdef VK_MAKE_API_VERSION
                                                                .apiVersion = VK_MAKE_API_VERSION (0, version_major, version_minor, 0),
#else
                                                                .apiVersion = VK_MAKE_VERSION (version_major, version_minor, 0),
#endif

[jpbruyere]

VK_MAKE_API_VERSION fix, is pushed with note on readme for min vulkan version, for the return values, it's on my todo list, thanks again.

[billzez]

Awesome, thanks. One more suggestion: Latest NDK defines VK_VERSION_1_2, even though few android devices support it, and you use this to set VKVG_VK_SCALAR_BLOCK_SUPPORTED. So with the latest NDK, I can't disable scalar block layout. Maybe the cmake setting to be to force disable scalar block?

[jpbruyere]

So I don't automatically enable scalar block if version = 1.2, but by default the option is enabled.

[jpbruyere]

I've pushed a fix #104

[jpbruyere]

Thank you for that report:-). Default layout fallback means c struct padding...

[jpbruyere]

I've made a fix in android_fix branch. I kept only compile time checks, handling 2 different structs at runtime don't look a too good option for me. If vulkan version < 1_2, I added a cmake option to force scalar block support. cmake > 3.23 will bring Vulkan_VERSION constant that we could use as option dependency.
I've not yet tested if vk version < 1_2.

[billzez]

Thanks. I was unable to test, the git submodule ref for vkh is broken in the android_fix branch.

[jpbruyere]

I've forgot to push vkh changes, done now, sorry.

[billzez]

Awesome thanks. More feedback:

1. VK_MAKE_VERSION takes three args:

    .apiVersion = VK_MAKE_VERSION (version_major, version_minor, 0)

2. error building with recording:

/root/vkvg/src/vkvg_context.c:613:2: error: non-void function 'vkvg_rectangle' should return a value [-Wreturn-type]
        RECORD(ctx, VKVG_CMD_RECTANGLE, x, y, w, h);

3. error building vkvg-svg:

/root/vkvg/external/vkvg-svg/src/parser_gen.h:5031:2: error: non-void function 'read_svg_attributes' should return a value [-Wreturn-type]
        ELEMENT_PRE_PROCESS_SVG
        ^
/root/vkvg/external/vkvg-svg/src/vkvg_svg.c:1799:3: note: expanded from macro 'ELEMENT_PRE_PROCESS_SVG'
                return;\

4. Lots of these warnings:

/root/vkvg/src/vkvg_surface.c:261:21: warning: expression result unused [-Wunused-value]
                LOG(VKVG_LOG_ERR, "vkvg_surface_write_to_png failed, invalid status: %d\n", surf->status);

5. Needs isnan_fix branch.

[jpbruyere]

ok, thanks. I'm aware of those problems with return values, I need to fix it (but it compile on my debian), For unused-value, I disable the warning, but maybe not on all platform (I'll double check it). Also I don't know if there is a better pattern for a macro disabling its content on release mode. For the make version I'll have to wrap both in a local macro I guess...Thank you for your help...

[jpbruyere]

return value bugs should be solved too, I've pushed all on the android_fix branch.

[billzez]

-DVKVG_RECORDING=false is currently broken:

/root/vkvg/src/vkvg_context.c: In function 'vkvg_rectangle':
/root/vkvg/src/vkvg_context.c:613:2: warning: implicit declaration of function 'RECORD2'; did you mean 'RECORD'? [-Wimplicit-function-declaration]
  613 |  RECORD2(ctx, VKVG_CMD_RECTANGLE, x, y, w, h);
      |  ^~~~~~~
      |  RECORD
/root/vkvg/src/vkvg_context.c:613:15: error: 'VKVG_CMD_RECTANGLE' undeclared (first use in this function)
  613 |  RECORD2(ctx, VKVG_CMD_RECTANGLE, x, y, w, h);

[billzez]

Disabling scalar block layout breaks gradients on my system. With or without scalar block enabled, the shader seems to expect:

typedef struct _vkvg_gradient_t {
        vkvg_color_t    colors[16];
        float                   stops[16];
        vec4                    cp[2];
        uint32_t                count;
}vkvg_gradient_t;

If I understand correctly, this layout should match both scalar and std430, but not std140. Could std430 always be specified?

[billzez]

I can confirm that if I use the struct above, use std430 instead of scalar in the shader, then its no longer necessary to enable the scalar block extension.

[jpbruyere]

I'm using #ifdef GL_EXT_scalar_block_layout in the shader to choose between the aligned or not structures, but because we made the use of scalar block layout independant from it's presence (we made it a compile option), I have to pass the vkvg option status to the shader on compilation. std430 still have alignments subtilities, I try my first option, if there's still problems, I will investigate your proposition..