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

Chunk version checking #4343

Merged
merged 11 commits into from
Jun 29, 2022
Merged

Chunk version checking #4343

merged 11 commits into from
Jun 29, 2022

Conversation

slimbuck
Copy link
Member

@slimbuck slimbuck commented Jun 20, 2022

Followup to #4254.

This PR adds a chunk system runtime compatibility check based on the engine version.

Applications use StandardMaterial.chunks.APIVersion to specify which engine version the overridden chunks implement. If any of the overridden chunks have a newer API defined in the engine (or the chunk was removed), a warning is printed to the debug console.

This helps app developers know which chunks have changed in their apps and in what version.

@slimbuck slimbuck added enhancement area: graphics Graphics related issue labels Jun 20, 2022
@slimbuck slimbuck requested a review from a team June 20, 2022 11:03
@slimbuck slimbuck self-assigned this Jun 20, 2022
@mvaligursky
Copy link
Contributor

  • Is there some public documentation to explain the user how to use the versioning?
  • When we print some of these 'incorrect version' messages, could we include a link to PR with the change, or engine release notes for the version or similar, to guide them in how to fix it?

@slimbuck
Copy link
Member Author

As part of engine v1.55 release I think we should start documenting chunk changes. Once we've decided where and how we can update the log message. (Though I am hesitant at the idea of printing URLs, seems a bit strange...).

Perhaps we can add a CHUNK_HISTORY.md file to the engine repo.

@willeastcott
Copy link
Contributor

From a quick glance, it doesn't look like all this new functionality is restricted to a debug build. Is that right?

@slimbuck
Copy link
Member Author

From a quick glance, it doesn't look like all this new functionality is restricted to a debug build. Is that right?

Yes the validation happens in all builds so that only valid chunks are used. The warning messages only appear in debug build.

@willeastcott
Copy link
Contributor

My concern is that this is going to bloat the release build over time as we continue to bump versions. Do we really have to carry around what is essentially a debugging aid in production builds of apps?

@mvaligursky
Copy link
Contributor

My concern is that this is going to bloat the release build over time as we continue to bump versions. Do we really have to carry around what is essentially a debugging aid in production builds of apps?

That's a good point. As long as the behaviour of the code with and without validation is the same (for example to always use user chunks, even when not up to date), we can drop the validation in release I think.

@slimbuck
Copy link
Member Author

Done. Also corrected chunk versions.

@Maksims
Copy link
Contributor

Maksims commented Jun 21, 2022

By not providing version property in chunks, it will lead to warnings? So pretty much every current developer will get a lot of warnings in their code after this update.

@slimbuck
Copy link
Member Author

By not providing version property in chunks, it will lead to warnings? So pretty much every current developer will get a lot of warnings in their code after this update.

@Maksims user will get warnings for chunks listed in chunk-validation.js, not all chunks. The message is only printed once and I have found them very useful.

@mvaligursky mvaligursky added the release: next minor Ticket marked for the next minor release label Jun 29, 2022
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 good to me.

@slimbuck slimbuck merged commit 02399fd into playcanvas:main Jun 29, 2022
@slimbuck slimbuck deleted the chunk-ver branch June 29, 2022 14:02
@slimbuck slimbuck mentioned this pull request Jun 30, 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 release: next minor Ticket marked for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants