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.

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

refactor material instance #5910

Merged
merged 13 commits into from
Dec 25, 2019
Merged

refactor material instance #5910

merged 13 commits into from
Dec 25, 2019

Conversation

zxg0622
Copy link
Contributor

@zxg0622 zxg0622 commented Dec 16, 2019

Re: cocos-creator/2d-tasks#

Changes:
*

@zxg0622
Copy link
Contributor Author

zxg0622 commented Dec 16, 2019

cocos-creator/editor-3d#2807

@zxg0622
Copy link
Contributor Author

zxg0622 commented Dec 18, 2019

cocos-creator/3d-tasks#623

@pandamicro
Copy link
Contributor

请修复 ci

@zxg0622
Copy link
Contributor Author

zxg0622 commented Dec 18, 2019

这个ci报错不是我的改动引起的

@pandamicro
Copy link
Contributor

rebase 一下就可以了

* @param index 材质序号
* @param material 材质对象
*/
public setMaterial (index: number, material: Material | null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

这么改有兼容问题啊,参数顺序变了原来代码都跑不了了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

编辑器支持api升级吗? @pandamicro

Copy link
Contributor

Choose a reason for hiding this comment

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

不支持用户层 API 升级,引擎侧需要做兼容,保留原有 API

@pandamicro
Copy link
Contributor

还需要再 rebase 一下

const source = parent.blocks[binding].view;
const dest = this._blocks[binding].view;
for (let j = 0; j < source.length; j++) {
dest[j] = source[j];
Copy link
Contributor

Choose a reason for hiding this comment

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

为啥要直接把 view 赋值过来?这样父子 pass UBO 就分不开了啊

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这是复制Float32Array里的元素,不是赋引用

else { Object.assign(defineOverrides, this._defines); defines = defineOverrides; }
}
const res = programLib.getGFXShader(this._device, this._programName, defines, pipeline);
const res = programLib.getGFXShader(this._device, this._programName, this._defines, pipeline);
if (!res.shader) { console.warn(`create shader ${this._programName} failed`); return null; }
if (saveOverrides) { this._shader = res.shader; this._bindings = res.bindings; }
Copy link
Contributor

Choose a reason for hiding this comment

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

这个函数就不用接受任何参数了吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里写得是有些问题,还需要再改下

import { IDefineMap, PassOverrides } from './pass';
import { PassInstance } from './pass-instance';

export class MaterialInstance implements IMaterial {
Copy link
Contributor

@YunHsiao YunHsiao Dec 23, 2019

Choose a reason for hiding this comment

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

这个类其实可以直接继承 Material,不然实在太多重复代码了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不直接继承Material的原因:
MaterialInstance不是一个Asset
MaterialInstance一部分属性是引用Material的,一部分属性是新建的

Copy link
Contributor

@YunHsiao YunHsiao Dec 23, 2019

Choose a reason for hiding this comment

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

Asset 那头只要不走基类的生命周函数,应该是没啥消耗的,只是一个概念上的问题
我看了,material-instance 引用父类的只有一个 effect asset,这个只是个引用,再生成一份也完全没问题,其他新建的属性如果继承的话可以完全沿用父类的机制

@@ -251,7 +234,6 @@ export class Material extends Asset {
this._states[passIdx] = overrides;
this._passes[passIdx].overridePipelineStates(passInfos[passIdx], overrides);
}
this._onPassesChange();
Copy link
Contributor

Choose a reason for hiding this comment

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

这俩函数可能需要在 material 层就报错了

@@ -229,7 +213,6 @@ export class Material extends Asset {
} else {
this._passes[passIdx].tryCompile(overrides);
}
this._onPassesChange();
Copy link
Contributor

Choose a reason for hiding this comment

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

这俩函数可能需要在 material 层就报错了

@YunHsiao
Copy link
Contributor

又看了下,其实 pass-instance 里也没复用多少数据,基本都是重建的,不如干脆都改继承了

@YunHsiao
Copy link
Contributor

不行就我来改这俩类,你改其他地方的引用

@zxg0622
Copy link
Contributor Author

zxg0622 commented Dec 23, 2019

model里面createPSO调用tryCompile的地方我用注释// warning:标注了,这个你回头改下 @YunHsiao

*/
// @property
get receiveShadows () {
Copy link
Contributor

Choose a reason for hiding this comment

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

涉及到对外 API 改动,必须写 deprecated 代码来确保兼容性,并做好必要的数据迁移

@property({
tooltip: '是否启用动态合批',
})
set enableDynamicBatching (enable: boolean) {
Copy link
Contributor

Choose a reason for hiding this comment

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

同上

@pandamicro pandamicro merged commit 39a7fbd into cocos:3d Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants