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

End game graph/log scale #1368

Merged
merged 6 commits into from
Nov 2, 2024

Conversation

John-194
Copy link
Contributor

Added log scale option.

Before:
Screenshot 2024-03-16 190918

After:
Screenshot 2024-03-16 190928

@sprunk
Copy link
Collaborator

sprunk commented Mar 16, 2024

Make a widget? I don't think this is the engine's job.

@John-194
Copy link
Contributor Author

John-194 commented Mar 16, 2024

Make a widget? I don't think this is the engine's job.

It isn't the engine's job but it is currently doing it. Making a widget requires me to learn lua, which I currently don't want to do and even then it will take a while to build everything from scratch.

@sprunk
Copy link
Collaborator

sprunk commented Mar 16, 2024

It isn't the engine's job but it is currently doing it.

Yes, but it's just there to start you out. Eventually you'll need to move to a Lua implementation anyway. If anything, it's a good argument to remove it and put a Lua implementation in basecontent since it doesn't really do anything vital. Perhaps the upcoming rmlui rewrite of native engine GUI would be an excellent opportunity to do that.

@John-194
Copy link
Contributor Author

John-194 commented Mar 16, 2024

It isn't the engine's job but it is currently doing it.

Yes, but it's just there to start you out. Eventually you'll need to move to a Lua implementation anyway. If anything, it's a good argument to remove it and put a Lua implementation in basecontent since it doesn't really do anything vital. Perhaps the upcoming rmlui rewrite of native engine GUI would be an excellent opportunity to do that.

So until rmlui gets implemented and the new graphs are made, maybe we can use this implementation? I would really like to view my graphs in log scale without having to wait for the new stuff (I believe other people would like that too), but if we must wait for the new stuff (who knows for how long), should I delete my PRs?

@sprunk
Copy link
Collaborator

sprunk commented Mar 16, 2024

I would really like to view my graphs in log scale without having to wait for the new stuff (I believe other people would like that too), but if we must wait for the new stuff (who knows for how long)

You don't have to wait for the new stuff, the Lua API to do so has been there for about 15 years and there's existing implementations (e.g. https://github.com/ZeroK-RTS/Zero-K/blob/master/LuaUI/Widgets/gui_chili_endgraph.lua ) that I would recommend stealing.

should I delete my PRs?

The other PR looks fine since it's just a tweak. To this one, normally I'd say yes but given that there is going to be a rewrite anyway I think we can accept it as well.

Copy link
Collaborator

@sprunk sprunk left a comment

Choose a reason for hiding this comment

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

There's a bit of bad whitespace all around but it probably doesn't matter if this is due a rewrite anyway.

@John-194
Copy link
Contributor Author

John-194 commented Mar 17, 2024

There's a bit of bad whitespace all around but it probably doesn't matter if this is due a rewrite anyway.

Form my end (VS Code), the whitespace looks consistent with the original code. Although when looking through GitHub's "Files changed", I can see some missing spaces, I don't know why that is.

@sprunk
Copy link
Collaborator

sprunk commented Mar 17, 2024

spaces vs tabs

i suppose i can fix that myself if i have a few minutes to spare before the merge window opens up

@Beherith
Copy link
Contributor

Base 10 log is overkill, I would choose either log2 or natural log.

Can the UV coordinates of the background images (graphpaper.bmp) be adjusted, and games can supply their graphpaperlogX.bmp to get logX tiling?

Handling all negative log10's as zero isn't very pretty for the non-resource type stuff. Does std::log handle the -inf correctly for values of zero?

Can any numbers of the graph be negative, where log isundef or NaN and the resulting clamping fails?

@sprunk sprunk self-requested a review March 18, 2024 01:01
@John-194
Copy link
Contributor Author

John-194 commented Mar 18, 2024

Base 10 log is overkill, I would choose either log2 or natural log.

The base doesn't matter because the graph and axis values get scaled accordingly to display correct values (not log).

Can the UV coordinates of the background images (graphpaper.bmp) be adjusted, and games can supply their graphpaperlogX.bmp to get logX tiling?

You mean something like this? Would probably take some extra time to implement non-linearly placed axis values.
image

Handling all negative log10's as zero isn't very pretty for the non-resource type stuff. Does std::log handle the -inf correctly for values of zero?
Can any numbers of the graph be negative, where log isundef or NaN and the resulting clamping fails?

It seems fine for zero, nan should throw an error although I didn't notice that. Should I place the v0 and v1 clamp (v0 = v0 <= 0.0f ? 0.0f : v0), before the logs and skip log calculation if zero?

@sprunk
Copy link
Collaborator

sprunk commented Sep 10, 2024

rmlUI has been merged so it would be ideal to move the graphs from engine into a rml widget.

@lhog
Copy link
Collaborator

lhog commented Nov 2, 2024

Guess we can merge it as is, it's not clear if and when it's going to be migrated to RML.

@sprunk
Copy link
Collaborator

sprunk commented Nov 2, 2024

Sure

@lhog lhog merged commit 7197dac into beyond-all-reason:master Nov 2, 2024
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.

4 participants