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

Shader documentation syntax fails to terminate with /**/ causing large editor freezes (regression 4.3-dev6) #91547

Closed
dog-on-moon opened this issue May 4, 2024 · 4 comments · Fixed by #91549

Comments

@dog-on-moon
Copy link
Contributor

Tested versions

  • Reproducible in 4.3-dev6
  • Not reproducible in 4.3-dev5

System information

Godot v4.3.dev6 - Windows 10.0.22631 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 2070 SUPER (NVIDIA; 31.0.15.4601) - Intel(R) Core(TM) i7-10700F CPU @ 2.90GHz (16 Threads)

Issue description

After swapping from 4.3-dev5 to 4.3-dev6, existing shader code from my project included the /**/ substring in various places. When the inspector attempts to process any shader containing this substring, there is a large lagspike (3 seconds with the MRP on my system).

This also causes the documentation syntax highlighting to affect the rest of the script. Perhaps caused by #90161 , related to #91424 by the appearance of the issue:

image

Steps to reproduce

  1. Create a CSGBox3D.
  2. Give it a StandardMaterial3D, convert it to ShaderMaterial.
  3. Add /**/ to the top of the code. (The amount of lag is proportional to the amount of code after the /**/.)
  4. Select and de-select the CSGBox3D. The editor will freeze with a large lagspike as it attempts to load the shader in the inspector.

Minimal reproduction project (MRP)

shader-syntax-lag.zip

@clayjohn clayjohn added this to the 4.3 milestone May 4, 2024
@clayjohn clayjohn moved this from Unassessed to Release Blocker in 4.x Release Blockers May 4, 2024
@clayjohn
Copy link
Member

clayjohn commented May 4, 2024

CC @magian1127

@magian1127
Copy link
Contributor

The CodeHighlighter::_get_line_syntax_highlighting_impl is determined to be flawed.
/**/ is considered the beginning of '/**'.
There are three solutions:

  1. Change the highlighting of /** to /** (one more space).
  2. Cancel the highlighting of /**.
  3. Rewrite the entire CodeHighlighter::_get_line_syntax_highlighting_impl.

I will first push a PR for solution 1.

@dalexeev
Copy link
Member

dalexeev commented May 4, 2024

In my opinion, we should revert the original PR and re-implement it using a parser instead of regular expressions. The * and + quantifiers can cause significant slowdowns due to backtracking.

@magian1127
Copy link
Contributor

The lagspike here is due to the fact that /**/ was blocking the regular expression /**.
The problem has been solved after changing it to /** .
I also think that a non-regular expression method is better, but it's still worth considering keeping the existing changes until there are new issues and new PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Release Blocker
Development

Successfully merging a pull request may close this issue.

4 participants