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

Make the ship's log bigger #1938

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

csibbitt
Copy link
Contributor

@csibbitt csibbitt commented Mar 11, 2023

Originally submitted as a naïve constant tweak that caused UI performance problems, UI usability problems, and network performance problems.

With @oznogon 's help and some additional work in the engine, these are all fixed. This patch depends on:

This should probably be a constant or more smart, but it bothered me now and this would fix the bother
@travishein
Copy link

👍

@oznogon
Copy link
Contributor

oznogon commented Mar 11, 2023

Running with a client in the server (single EmptyEpsilon.exe instance), 8-core/16-thread Ryzen 9/RTX 3060, with a script that sent 100 entries upon scenario launch and one every 0.1 seconds after:

dock1 = PlayerSpaceship():setTemplate("Atlantis"):setPosition(-100, 0):setFaction("Human Navy")
i = 1
timeout = 0.1

for j = 1, 100, 1
do
    dock1:addToShipLog("Ack" .. i, "yellow")
    i = i + 1
end

function update(delta)
    timeout = timeout - delta
    if(timeout < 0)
    then
        dock1:addToShipLog("Ack" .. i, "yellow")
        i = i + 1
        timeout = 0.1
    end
end

By around 1,000 entries, the GuiSlider lozenge becomes too small to reliably use with touch:

image

By around 1,500 entries, the log begins to cause fps to dip below 60 on the Ship's Log screen.

By around 2,000 entries, the lozenge becomes too small to click.

image

By around 2,750 entries, the client is sending 500kb over the wire per second.

By around 3,000 entries, FPS drops to below 30fps.

image

At 3,500 entries, CPU fans start to spin up to handle the load and FPS drops to under 25.

image

At 4,000 entries, FPS is at 21 and the lozenge is nearly obscured by the scrollbar arrow.

image

At 4,500 entries we start flirting with 1MB over the wire per second.

image

Resource Monitor confirms this.

image

At this point I open up a client over the network on Relay.

With the Ship's Log minimized on Relay, we still get 140+ fps.

With it expanded, fps drops to 15.

image

I had to stop the server around 7,000 entries because my CPU was pinned and reporting 90C temps. FPS was around 12 and system performance outside of EE was beginning to be impacted.

  • GuiSlider needs a minimum touch-friendly lozenge size
  • Viewing the Ship's Log with large numbers of entries should not impact client or server fps
  • At some point for network replication, log entries needs to be paginated, cacheable, or truncated for clients

@oznogon
Copy link
Contributor

oznogon commented Mar 11, 2023

The ship log (the entire log, I think) is replicated to clients every frame. Not sure but I think this means it's replicated even to players who aren't using Relay/Ship's Log/any screen with access to the log?

registerMemberReplication(&ships_log);

A dirty fix would reduce the replication frequency to 1.0 (1 second) or so. A better fix would send the entire log once to a new client and only send updates after that, but that would be a more dramatic implementation change.

@oznogon
Copy link
Contributor

oznogon commented Mar 11, 2023

Tried sending 10000 entries to the player upon launch, instead of sending entries constantly, and I'm not seeing the replication. Still not great if each new entry replicates the entire log. Either way, FPS with the ship's log visible drops to <9 with 10,000 entries and stays there. The GuiScrollbar lozenge looks like it's 1px on load but doesn't appear to render if you scroll anywhere into the log.

image

@daid Does the log get replicated each frame or just on changes?

@oznogon
Copy link
Contributor

oznogon commented Mar 11, 2023

daid via Discord:

yes, it's extremely inefficient, which is why it is so limited in size
it's not just that the whole log gets sent, it's also checking every entry for changes each tick.
... I'm planning to change this on with the ECS code

Re: GUI performance issues with >1,500 entries:

My guess, the minimized (view in Relay) just grabs the last line and displays that. But the expanded version first makes a "preparedfontrenderer" (call) for the all the lines, so it knows how much vertical space there is, and then draws a subsection of that.

@oznogon
Copy link
Contributor

oznogon commented Mar 12, 2023

Found another GuiScrollbar limitation: after 400 entries, the bottom entries in the log can't be viewed by scrolling, and the granularity of scrolling via scrollbar arrows is 1px/click, or too little to be useful. #1940 (comment), #1941, #1942

@csibbitt
Copy link
Contributor Author

@oznogon Thank you very much for testing this. I was expecting someone to explain the history of why it is like this, possibly with these sorts of details, but you did it with science! It seems we have at least a UI issue and a performance issue to deal with before this could become a reality. I'll check out the discord discussion and see what I can do to help.

csibbitt and others added 6 commits October 7, 2023 16:02
This is before insertion, so >400 is 401 lines. I noticed this when I set it to 10 for a different experiment and saw 11 lines.
* sp::SeqVector provides efficient network sync
* sequence #s must start at 1 for replication
@csibbitt
Copy link
Contributor Author

Not sure if the CI system supports dependent changes, but this definitely won't even compile until daid/SeriousProton#239 merges, and is not very usable until the other two required changes merge.

@csibbitt
Copy link
Contributor Author

With all of the changes in place, I was able to run the tests described here: daid/SeriousProton#239 (comment)

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