Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

shadertools: Resolve dependencies in ShaderInputs #2073

Merged
merged 3 commits into from
Apr 25, 2024
Merged

shadertools: Resolve dependencies in ShaderInputs #2073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cd2035
Resolve dependencies in ShaderInputs
felixpalmer Apr 16, 2024
28df7ca
Revert back to using _resolveModules
felixpalmer Apr 18, 2024
217aef3
Typing
felixpalmer Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions modules/engine/src/shader-inputs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,22 @@ export class ShaderInputs<
* @param modules
*/
// @ts-expect-error Fix typings
constructor(modules: {[P in keyof ShaderPropsT]: ShaderModuleInputs<ShaderPropsT[P]>}) {
// TODO - get all dependencies from modules
const allModules = _resolveModules(Object.values(modules));
log.log(
1,
'Creating ShaderInputs with modules',
allModules.map(m => m.name)
)();
constructor(modules: {[P in keyof ShaderPropsT]?: ShaderModuleInputs<ShaderPropsT[P]>}) {
// Extract modules with dependencies
const resolvedModules = _resolveModules(
Object.values(modules).filter(module => module.dependencies),
false
);
for (const resolvedModule of resolvedModules) {
// @ts-expect-error Fix typings
modules[resolvedModule.name] = resolvedModule;
}

log.log(1, 'Creating ShaderInputs with modules', Object.keys(modules))();

// Store the module definitions and create storage for uniform values and binding values, per module
this.modules = modules;
// @ts-expect-error Fix typings
this.modules = modules as {[P in keyof ShaderPropsT]: ShaderModuleInputs<ShaderPropsT[P]>};
this.moduleUniforms = {} as Record<keyof ShaderPropsT, Record<string, UniformValue>>;
this.moduleBindings = {} as Record<keyof ShaderPropsT, Record<string, Texture | Sampler>>;

Expand Down
31 changes: 31 additions & 0 deletions modules/engine/test/shader-inputs.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import test from 'tape-promise/tape';
import {picking} from '../../shadertools/src/index';
// import {_ShaderInputs as ShaderInputs} from '@luma.gl/engine';
import {ShaderInputs} from '../src/shader-inputs';
import {ShaderModule} from '@luma.gl/shadertools';

test('ShaderInputs#picking', t => {
const shaderInputsUntyped = new ShaderInputs({picking});
Expand Down Expand Up @@ -62,3 +63,33 @@ test('ShaderInputs#picking prop merge', t => {

t.end();
});

test('ShaderInputs#dependencies', t => {
type CustomProps = {color: number[]};
const custom: ShaderModule<CustomProps> = {
name: 'custom',
dependencies: [picking],
uniformTypes: {
color: 'vec3<f32>'
}
};

const shaderInputs = new ShaderInputs<{
custom: CustomProps;
picking: typeof picking.props;
}>({custom});
t.deepEqual(Object.keys(shaderInputs.modules), ['custom', 'picking']);

shaderInputs.setProps({
custom: {color: [255, 0, 0]},
picking: {highlightedObjectColor: [1, 2, 3]}
});
t.deepEqual(shaderInputs.moduleUniforms.custom.color, [255, 0, 0], 'custom color updated');
t.deepEqual(
shaderInputs.moduleUniforms.picking.highlightedObjectColor,
[1, 2, 3],
'highlight object color updated'
);

t.end();
});
37 changes: 28 additions & 9 deletions modules/shadertools/src/lib/shader-assembly/resolve-modules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,34 @@ import type {ShaderModule} from '../shader-module/shader-module';
import {ShaderModuleInstance} from '../shader-module/shader-module-instance';

/**
* Instantiate shader modules and esolve any dependencies
* Resolve any dependencies and optionally instantiate modules
*/
export function resolveModules(
modules: (ShaderModule | ShaderModuleInstance)[]
): ShaderModuleInstance[] {
const instances = ShaderModuleInstance.instantiateModules(modules);
return getShaderDependencies(instances);
): ShaderModuleInstance[];
export function resolveModules(
modules: (ShaderModule | ShaderModuleInstance)[],
instantiate: true
): ShaderModuleInstance[];
export function resolveModules(modules: ShaderModule[], instantiate: false): ShaderModule[];
export function resolveModules(
modules: ShaderModuleInstance[],
instantiate: false
): ShaderModuleInstance[];
export function resolveModules(
modules: (ShaderModule | ShaderModuleInstance)[],
instantiate: boolean = true
): (ShaderModule | ShaderModuleInstance)[] {
return getShaderDependencies(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A problem here is that if a ShaderModule has been involved in instantiation, all its dependencies have been converted to ShaderModuleInstances. So I am not convinced that the result of any repeated calls to resolveModules() will actually be a ShaderModule[] in spite of this typing effort.

The ShaderModule / ShaderModuleInstance dichotomy seems to generate a lot more trouble than it is worth.

I note that instantiation actually doesn't do that much, it adds some methods (that could be global functions) and initializes some things for perf:

    this.deprecations = this._parseDeprecationDefinitions(deprecations);
    this.injections = normalizeInjections(inject);
    if (uniformPropTypes) {
      this.uniforms = makePropValidators(uniformPropTypes);
    }
}

Perhaps we should try to move away from ShaderModuleInstance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to remove ShaderModuleInstance so that we can avoid this mess, partial work in #2074 will continue tomorrow

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, it also makes it simpler for situations where we extend ShaderModule, like ShaderPass. Why have ShaderModuleInstance, but not ShaderPassInstance? If I instantiateModules with ShaderPass instances what is returned etc...

instantiate === false ? modules : ShaderModuleInstance.instantiateModules(modules)
);
}

type AbstractModule = {
name: string;
dependencies?: AbstractModule[];
};

/**
* Takes a list of shader module names and returns a new list of
* shader module names that includes all dependencies, sorted so
Expand All @@ -27,8 +46,8 @@ export function resolveModules(
* @param modules - Array of modules (inline modules or module names)
* @return - Array of modules
*/
function getShaderDependencies(modules: ShaderModuleInstance[]): ShaderModuleInstance[] {
const moduleMap: Record<string, ShaderModuleInstance> = {};
function getShaderDependencies<T extends AbstractModule>(modules: T[]): T[] {
const moduleMap: Record<string, T> = {};
const moduleDepth: Record<string, number> = {};
getDependencyGraph({modules, level: 0, moduleMap, moduleDepth});

Expand All @@ -48,10 +67,10 @@ function getShaderDependencies(modules: ShaderModuleInstance[]): ShaderModuleIns
* @return - Map of module name to its level
*/
// Adds another level of dependencies to the result map
export function getDependencyGraph(options: {
modules: ShaderModuleInstance[];
export function getDependencyGraph<T extends AbstractModule>(options: {
modules: T[];
level: number;
moduleMap: Record<string, ShaderModuleInstance>;
moduleMap: Record<string, T>;
moduleDepth: Record<string, number>;
}) {
const {modules, level, moduleMap, moduleDepth} = options;
Expand Down
Loading