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

Remove unused auto instancing code #4060

Merged
merged 2 commits into from
Mar 1, 2022

Conversation

erikdubbelboer
Copy link
Contributor

Code doesn't do anything and was left from the past.

Removing the enableAutoInstancing getter and setter could be a breaking change for users still calling these methods (even though they don't do anything).

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

Code doesn't do anything and was left from the past.

Removing enableAutoInstancing getter and setter could be a breaking
change for users still calling these methods (even though they
don't do anything).
@mvaligursky
Copy link
Contributor

I looked at this few times before, but always decided to keep this in, as I'm keen to implement auto instancing / auto batching at some point. Auto batching would be particularly useful for UI rendering.

@erikdubbelboer
Copy link
Contributor Author

Isn't it better to just remove it now and in the future have a single PR which adds all the code required for this? It will probably work differently anyways.

@willeastcott
Copy link
Contributor

I'm inclined to agree. I mean, instancing is not down to be worked on in the next few months while we focus on WebGPU etc. If this stuff genuinely doesn't currently work and isn't being used anywhere, I'd say just remove it. But just my humble opinion.

@mvaligursky
Copy link
Contributor

I'm fine with removing it.
With this changed code now, drawInstance functions now all returns 0 I believe, we should stop them from returning anything, and clean up how they get called as well to not use the return value.

Otherwise it looks good .. I was worried Editor would need to be updated to hide those stats from it, but those are not exposed anyways.

@erikdubbelboer
Copy link
Contributor Author

Good point, I'll make those changes tomorrow.

@erikdubbelboer
Copy link
Contributor Author

It's fixed.

Copy link
Contributor

@mvaligursky mvaligursky left a comment

Choose a reason for hiding this comment

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

Looks great to me, nicely done!

@mvaligursky mvaligursky merged commit 9184929 into playcanvas:dev Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants