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

Fix shader preprocessor cyclic include handling #77608

Merged
merged 1 commit into from
May 30, 2023

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented May 29, 2023

Fixes #75813

Normally, shader include dependencies are calculated starting from the root shader, but when editing a shader include directly in the editor the depencencies are not always fully known until a full compile is done. This PR adds a success check to preprocessing step and only updates dependencies if the preprocessing worked. Now a proper cyclic dependency error message is shown and the engine doesn't crash.

While investigating this I also made a few more minor fixes:

  1. Use const references in range loop iterators to avoid excessive copying of Ref-objects.
  2. Fix an off-by-one line numbering error highlight in some error cases.
  3. Add the name of the file to the error message when reporting a cyclic include error.
  4. Add forgotten file name when reporting certain errors to the editor.

@bitsawer bitsawer requested a review from a team as a code owner May 29, 2023 10:51
@YuriSizov YuriSizov modified the milestones: 4.x, 4.1 May 29, 2023
Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

LGTM, AFAICT. OTOH, TIWAGOS.

  • TIWAGOS stands for Take it [my approval] with a grain of salt.

Copy link
Member

@clayjohn clayjohn 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 too

@YuriSizov YuriSizov merged commit faa73c9 into godotengine:master May 30, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@bitsawer bitsawer deleted the fix_cyclic_includes branch May 30, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cyclic Dependencies in shader freeze
4 participants