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 _get_debug_tooltip crash if tooltip string is too large #81018

Conversation

jsjtxietian
Copy link
Contributor

@jsjtxietian jsjtxietian commented Aug 26, 2023

This pr solves part of #80959

There are three problems raised from this issue, this pr only deals with the problem that if the stack variable's tooltip string is smaller than 1Mb ( if bigger than 1Mb, the case is addressed by another pr) but still very big, vulkan will crash at creating the buffer for the tooltip's flat style box since it's too long.

I choose to truncate the string at position 200 and append a ' [...] overflowed!' at the end of the string. The number is arbitrary but I dont think it's necessary to expose the value for user to config.

@jsjtxietian jsjtxietian requested a review from a team as a code owner August 26, 2023 15:45
@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from 0ef617c to e46b31e Compare August 26, 2023 15:48
@AThousandShips AThousandShips added this to the 4.2 milestone Aug 26, 2023
@bitsawer
Copy link
Member

Related / Alternative PR:

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Aug 26, 2023

Oh I missed that, but there is on other thing I can do to help this pr.

@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from e46b31e to eb1c773 Compare August 26, 2023 16:18
@jsjtxietian jsjtxietian requested a review from a team as a code owner August 26, 2023 16:18
@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from eb1c773 to a602682 Compare August 26, 2023 16:26
@jsjtxietian jsjtxietian changed the title Return more user friendly info in ScriptStackVariable::serialize instead of nil Fix _get_debug_tooltip will crash if tooltip string is too large Aug 26, 2023
@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch 2 times, most recently from abf7e19 to 93203dc Compare August 26, 2023 16:29
@YuriSizov YuriSizov changed the title Fix _get_debug_tooltip will crash if tooltip string is too large Fix _get_debug_tooltip crash if tooltip string is too large Aug 31, 2023
@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from 93203dc to 9854f31 Compare September 1, 2023 04:20
@miv391
Copy link
Contributor

miv391 commented Sep 1, 2023

I would suggest word "truncated" instead of "overflowing". Overflow sounds like an error to me.

@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch 4 times, most recently from a2ca2f5 to 8e19994 Compare September 2, 2023 04:42
@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from 8e19994 to 28d8251 Compare November 3, 2023 09:46
@YuriSizov
Copy link
Contributor

I think that warning is excessive, and will end up spamming users that hover over some long values. Nothing they can do about it either, so it's not very useful.

I also think 200 is a very short threshold? What kind of content do we expect there?

@YuriSizov YuriSizov added crash cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release and removed performance labels Nov 3, 2023
@YuriSizov YuriSizov modified the milestones: 4.2, 4.3 Nov 3, 2023
@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Nov 4, 2023

I also think 200 is a very short threshold? What kind of content do we expect there?

Just the content shown when you hover over a gdscript variable. The crash I tried to fix comes from a extremely large json map, the whole content of this map will crash vulkan since the tooltip box is too long, vma can't allocate so much big.

@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from 28d8251 to 0867a27 Compare November 6, 2023 02:32
@YuriSizov
Copy link
Contributor

@jsjtxietian Well, yes, I understand that. But I assume that a very large JSON is far beyond 200 characters? I'm just trying to see if this is not an overzealous threshold.

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Nov 6, 2023

a very large JSON is far beyond 200 characters

Yes, > 1w characters.

I'm just trying to see if this is not an overzealous threshold.

It maybe too small, but as far as I can see, a tooltip longer than several dozens is already hard too read. 200 is just an arbitary number I chose, it can be changed. Just a safe guard to prevent vulkan's crash.

I feel the ultimate solution for this should be a tooltip like what you can get from visual studio (rather than just a tip) , you can expand it to see the whole content.

@Calinou
Copy link
Member

Calinou commented Nov 6, 2023

I think this threshold can be extended to 250 or even 300, but I'm fine with the idea in principle. Remember to test this at high editor scales too, as the issue may occur earlier at high editor scales.

@jsjtxietian
Copy link
Contributor Author

I think this threshold can be extended to 250 or even 300, but I'm fine with the idea in principle. Remember to test this at high editor scales too, as the issue may occur earlier at high editor scales.

Tested locally, 300 with scale at 200% is fine.

@YuriSizov YuriSizov modified the milestones: 4.3, 4.2 Nov 6, 2023
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Nov 6, 2023
@YuriSizov
Copy link
Contributor

Tested locally, 300 with scale at 200% is fine.

Not that it matters much whether it's 200 or 300, but let's go with that, I guess :) Could you also put the number into a local constant so it's easier to modify if needs be and so it's less magical?

@jsjtxietian
Copy link
Contributor Author

jsjtxietian commented Nov 6, 2023

Oh another thing, I just found that we can use '\n' to seperate strings so they won't become too long, should I do something like , let's say after every 50 characters, insert a '\n' into it.

@YuriSizov
Copy link
Contributor

I think for now plugging the crash is what's important. Improving usability or solving this in a better way can be done later.

@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from 0867a27 to 2e715bb Compare November 6, 2023 12:05
@jsjtxietian jsjtxietian force-pushed the return-use-friendly-info-in-scriptStackVariable-serialize branch from 2e715bb to 155d738 Compare November 6, 2023 12:09
@YuriSizov YuriSizov added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Nov 6, 2023
@YuriSizov YuriSizov merged commit 14cc639 into godotengine:master Nov 6, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@jsjtxietian jsjtxietian deleted the return-use-friendly-info-in-scriptStackVariable-serialize branch November 6, 2023 15:35
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

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.

6 participants