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

Version checking for crash dumper #617

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Buckminsterfullerene02
Copy link
Member

@Buckminsterfullerene02 Buckminsterfullerene02 commented Aug 11, 2024

Description

We keep getting issues posted where the user is using an old version of UE4SS that does not work for their game.

Yangff suggested adding information that checks for new versions when crash.

Currently this change is adding the information in the message box that opens when it makes a crash dump, however the code is exposed in UE4SSProgram.hpp so could be used in other places such as in the logs or somewhere more obvious to the user, and it seems useful enough to add in the mod APIs in the future.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tested output of warning by temporarily flipping if (!is_latest_ue4ss_version(latest_version)) condition.

Checked that config works as intended.

Need to force crash dump to happen to test crash dumper part.

Checklist

  • I have commented my code, particularly in hard-to-understand areas.
  • I have added the necessary description of this PR to the changelog, and I have followed the same format as other entries.

@Buckminsterfullerene02
Copy link
Member Author

Draft PR because code currently does not build due to a bunch of seemingly unrelated errors in LiveView.cpp and Dumpers.cpp. @UE4SS any ideas?

Copy link
Contributor

github-actions bot commented Aug 11, 2024

MSVC-Game__Debug__Win64 Download Logs
Build Details
Name Information
PR Commit dfcd94b
Merge Commit dbeb398
Size 43 MB
Last Updated Aug 12, 24, 5:01:07 PM UTC
Expires At Aug 26, 24, 5:01:02 PM UTC

MSVC-Game__Shipping__Win64 Download Logs
Build Details
Name Information
PR Commit dfcd94b
Merge Commit dbeb398
Size 26.23 MB
Last Updated Aug 12, 24, 5:03:59 PM UTC
Expires At Aug 26, 24, 5:03:56 PM UTC

@UE4SS
Copy link
Collaborator

UE4SS commented Aug 12, 2024

Draft PR because code currently does not build due to a bunch of seemingly unrelated errors in LiveView.cpp and Dumpers.cpp. @UE4SS any ideas?

This is caused by curl.h either defining the max macro or including something that does.
Perhaps it leaks Windows.h, this is usually the root of this problem.
The solution is to move the curl.h include to the cpp file.

@UE4SS
Copy link
Collaborator

UE4SS commented Aug 12, 2024

Just fyi, you don't need to put all functions in the header.
Unless you want to call the function outside UE4SSProgram.cpp, or you want other people to be able to call the function, it shouldn't go in the header because it's more difficult to find what you're looking for if the header is big and bloated, and this is especially problematic if it's an exported header meant for other people (mods) to use.
The is and get functions make sense to put in the header, but the rest of them seem like internal details that no one is going to need to know exists.
Static free functions in the .cpp file is what should be used for internal functions unless they absolutely need private access to a class.

It's not a blocking problem, but it's something to keep in mind, at least for future PRs.

@Buckminsterfullerene02
Copy link
Member Author

Buckminsterfullerene02 commented Aug 12, 2024

Need to force crash dump to happen to test crash dumper part. I tried dereferincing null ptr, accessing index out of range of array and infinite recursion, and none of them cause the crash (I put breakpoint on and they are running that code as marked by the tick on breakpoint icon) so I am out of ideas of ways to force a crash. I swear I've done this before successfully...

@Buckminsterfullerene02
Copy link
Member Author

Just fyi, you don't need to put all functions in the header. Unless you want to call the function outside UE4SSProgram.cpp, or you want other people to be able to call the function, it shouldn't go in the header because it's more difficult to find what you're looking for if the header is big and bloated, and this is especially problematic if it's an exported header meant for other people (mods) to use. The is and get functions make sense to put in the header, but the rest of them seem like internal details that no one is going to need to know exists. Static free functions in the .cpp file is what should be used for internal functions unless they absolutely need private access to a class.

It's not a blocking problem, but it's something to keep in mind, at least for future PRs.

I wasn't even aware you could do this, mind showing an example? Or doing it with write_callback or split_version yourself to show me for as example.

@UE4SS
Copy link
Collaborator

UE4SS commented Aug 12, 2024

Just fyi, you don't need to put all functions in the header. Unless you want to call the function outside UE4SSProgram.cpp, or you want other people to be able to call the function, it shouldn't go in the header because it's more difficult to find what you're looking for if the header is big and bloated, and this is especially problematic if it's an exported header meant for other people (mods) to use. The is and get functions make sense to put in the header, but the rest of them seem like internal details that no one is going to need to know exists. Static free functions in the .cpp file is what should be used for internal functions unless they absolutely need private access to a class.
It's not a blocking problem, but it's something to keep in mind, at least for future PRs.

I wasn't even aware you could do this, mind showing an example? Or doing it with write_callback or split_version yourself to show me for as example.

It's as simple as just creating the function in the cpp file, as long as the function is declared before it's called.
Here's an example:

// UE4SSProgram.hpp
class UE4SSProgram
{
    auto some_member_function() -> void;
};

// UE4SSProgram.cpp
// This function can only be called if it's defined before the caller is defined.
// Which means you have to worry about the ordering of functions in the file.
static auto function_name() -> int
{
    return 4;
}

// This prevents you from having to worry about the order too much.
// Just put this declaration somewhere far up the file.
// The definition (function body) can be placed anywhere, including after the caller.
static auto other_function() -> void;

auto UE4SSProgram::some_member_function() -> void
{
    function_name();
    other_function();
}

static auto other_function() -> void
{
    // logic for this function
}

if not latest ue4ss version from github, add to crash dumper messagebox

use xrepo libcurl

use curl redirect to avoid requiring user agent for api

add setting for check
chore: changelog
@igromanru
Copy link
Contributor

Personally, I find the feature unnecessary. Isn't it the first feature that requires an internet connection?
When a single game suddenly asks for internet permissions, I'm always suspicious and I have to look in the firewall what it actually does. That's why I'm not a fan of such things, unless it's necessary.

We keep getting issues posted where the user is using an old version of UE4SS that does not work for their game.

At current state a lot of games require the latest experimental version to work, in such case it's not helpful. Also a lot of users don't even read the log themself, they just post "it doesn't work with game XYZ".
If they aren't even smart enough to try out the latest Release version before posting an issue nothing will help.

@UE4SS
Copy link
Collaborator

UE4SS commented Oct 6, 2024

Personally, I find the feature unnecessary. Isn't it the first feature that requires an internet connection? When a single game suddenly asks for internet permissions, I'm always suspicious and I have to look in the firewall what it actually does. That's why I'm not a fan of such things, unless it's necessary.

We keep getting issues posted where the user is using an old version of UE4SS that does not work for their game.

At current state a lot of games require the latest experimental version to work, in such case it's not helpful. Also a lot of users don't even read the log themself, they just post "it doesn't work with game XYZ". If they aren't even smart enough to try out the latest Release version before posting an issue nothing will help.

I think your points here are definitely valid, and I agree to some extent.
I don't think we should check for a new version on startup, because no one checks the log and a message box would just get in the way, but I think checking when it crashes and displaying it in the preexisting error message box is fine.
Currently it checks on startup and when it crashes.

The feature itself obviously requires an internet connection, but if there isn't a connection, as far as I know it will currently fall to the "a new version is available" case, which isn't great and it would be good if this was changed if this PR goes forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants