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

Fixed error spam in remote debugger #22201

Merged
merged 1 commit into from
Sep 20, 2018
Merged

Fixed error spam in remote debugger #22201

merged 1 commit into from
Sep 20, 2018

Conversation

Piet-G
Copy link
Contributor

@Piet-G Piet-G commented Sep 18, 2018

Fixed error spam in remote debugger.

This finally fixes #20365 for real this time.

Big thanks to @willnationsdev for tracking down where it all went wrong.

@Piet-G Piet-G requested a review from reduz as a code owner September 18, 2018 00:10
@akien-mga akien-mga added this to the 3.1 milestone Sep 18, 2018
@akien-mga
Copy link
Member

This RES: prefix was added by @Geequlim in ccf7679, to be used in

if (hint_string.begins_with("RES:") && hint_string != "RES:") {
String path = hint_string.substr(4, hint_string.length());
var = ResourceLoader::load(path);
}

@Piet-G
Copy link
Contributor Author

Piet-G commented Sep 18, 2018

Ah yes I see, I overlooked that. I'll see what I can do about that.

@willnationsdev
Copy link
Contributor

@dualmatrix You can probably just add a delimeter in the hint string to store both values and then split it up on the other side.

@Piet-G
Copy link
Contributor Author

Piet-G commented Sep 18, 2018

Fixed it while preserving the resource path!

if (is_new_object) {
//don't update.. it's the same, instead refresh
debugObj->prop_list.push_back(pinfo);
}

if (Variant::get_type_name(var.get_type()) == "String") {
Copy link
Member

Choose a reason for hiding this comment

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

It would be cleaner to check var.get_type() == Variant::STRING IMO instead of checking the human readable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah thats better, fixed!

@akien-mga
Copy link
Member

akien-mga commented Sep 19, 2018

I'm not convinced by the fix overall, not because it's bad, but because it doesn't improve the existing hacky code that caused the issue in the first place.

I'm not familiar with this code but I think it might be possible to find a cleaner way to identify resources that have to be loaded as a path, without abusing either the property hint or the Variant value itself.

Input from @Geequlim would be welcome.

Fixed error spam in remote debugger.
@Piet-G
Copy link
Contributor Author

Piet-G commented Sep 19, 2018

Yeah it isn't the cleanest code, but it's at least less hacky/broken than previously where the path was stored where the type of the resource was supposed to be, storing it in var at least kinda makes sense imo.
But if someone wants to rewrite this to make it cleaner please go ahead, I don't really feel confident enough to do that.

@akien-mga
Copy link
Member

As you say, that's good enough for now. The debugger protocol might need a cleanup, but as long as it works... :)

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.

"core\class_db.cpp:324 - Condition ' !ti ' is true. returned: StringName()" is spammed to console.
3 participants