godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
85.18k stars 18.72k forks source link

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

Closed fauxhaus closed 2 weeks ago

fauxhaus commented 4 weeks ago

Tested versions

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 commented 4 weeks ago

CC @magian1127

magian1127 commented 4 weeks ago

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 commented 4 weeks ago

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 commented 4 weeks ago

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.