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

Frontend Speedup #2183

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

Frontend Speedup #2183

wants to merge 9 commits into from

Conversation

darkfated
Copy link
Contributor

Performance Boost for Frontend

  • HTML Optimizations: Moved JS script loading after HTML to improve page rendering.
  • Lazy Loading for Images: Implemented defer for images that load only under specific conditions.
  • jQuery Update: Updated jQuery to the latest version for better stability.
  • DOMContentLoaded Event: Changed the event for loading the menu to DOMContentLoaded, improving rendering speed and response time.
  • jQuery Refactoring: Reduced reliance on jQuery, replacing it with plain JavaScript to enhance performance and reduce overhead.

The jQuery update was tested, and no issues were detected. All existing functionality remains fully operational, and the overall performance has slightly improved with the new version.

@DarthTealc
Copy link
Contributor

This looks good, if it works in Awesomium.

There are a few places you could reduce jQuery reliance further while still working in Awesomium if you wanted. Some examples on loading.html replacing jQuery with vanilla javascript

  • Any first-match element selections eg $("#serverName") could use document.querySelector("#serverName")
  • Text value setting eg .text( servername ); could use .innerText = servername;
  • Attribute setting eg .attr("src", "asset://mapimage/" + mapname ); could use .setAttribute("src", "asset://mapimage/" + mapname);
  • CSS setting eg .css( "background-image", "url( 'asset://mapimage/" + mapname + "' )" ); could use .style.backgroundImage = "url( 'asset://mapimage/" + mapname + "' )";
  • Adding classes eg .addClass( "visible" ); could use .classList.add( "visible" );

@darkfated
Copy link
Contributor Author

This looks good, if it works in Awesomium.

There are a few places you could reduce jQuery reliance further while still working in Awesomium if you wanted. Some examples on loading.html replacing jQuery with vanilla javascript

  • Any first-match element selections eg $("#serverName") could use document.querySelector("#serverName")
  • Text value setting eg .text( servername ); could use .innerText = servername;
  • Attribute setting eg .attr("src", "asset://mapimage/" + mapname ); could use .setAttribute("src", "asset://mapimage/" + mapname);
  • CSS setting eg .css( "background-image", "url( 'asset://mapimage/" + mapname + "' )" ); could use .style.backgroundImage = "url( 'asset://mapimage/" + mapname + "' )";
  • Adding classes eg .addClass( "visible" ); could use .classList.add( "visible" );

Awesomium is working. Yes, your suggestions are good

Copy link
Contributor

@DarthTealc DarthTealc left a comment

Choose a reason for hiding this comment

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

I checked out these changes briefly in both x86-64 and Main and I didn't experience any regressions. Everything seems to work without issue. I'd endorse merging.

Only thing I wonder is if we even still need jquery-color.js and jquery-ui.js, and if so, could they be replaced with lightweight standard jquery use or vanilla javascript. But that's probably worth doing as a separate pull request.

@robotboy655 robotboy655 added the Enhancement The pull request enhances current functionality. label Jan 17, 2025
@robotboy655
Copy link
Collaborator

robotboy655 commented Jan 17, 2025

So about this whole "it's been tested" thing..
image

Edit: This appears to only happen on first launch with the changes applied.

@robotboy655
Copy link
Collaborator

[HTML] asset://garrysmod/html/js/menu/control.Main.js?2:88: Uncaught TypeError: Cannot call method 'SetHideNewsList' of null

I suspect your JS loading order changes cause race conditions everywhere.

@robotboy655
Copy link
Collaborator

These changes also create an ugly effect on game start up where the menu is half-baked in the middle of the screen for a split second.

image

Copy link
Collaborator

@robotboy655 robotboy655 left a comment

Choose a reason for hiding this comment

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

See above.

The code style in the JS changes also does not match the rest of the code base.

@darkfated
Copy link
Contributor Author

[HTML] asset://garrysmod/html/js/menu/control.Main.js?2:88: Uncaught TypeError: Cannot call method 'SetHideNewsList' of null

I suspect your JS loading order changes cause race conditions everywhere.

Yes, that was the problem. For the test, I reinstalled the game and tried to cause bugs - no problems were found now

Desktop.2025.01.18.-.00.20.44.05.mp4

@darkfated darkfated requested a review from robotboy655 January 17, 2025 21:33
@robotboy655
Copy link
Collaborator

Testing the latest changes, I am still encountering every single bug I already mentioned above.

I think the only way to move forward here is to remove defer from the scripts and move them back to where they were.

Maybe defer could work for the sub menus, but not for the front page.

I am testing like this:

  1. delete old html folder
  2. copy in new one
  3. Launch the game, and see bugs
  4. remove new html/ folder, copy over the old one
  5. launch the game, everything works
  6. go to step 1

I think this simulates a cold boot of the game or something, resetting whatever cache is there.

@darkfated
Copy link
Contributor Author

Testing the latest changes, I am still encountering every single bug I already mentioned above.

I think the only way to move forward here is to remove defer from the scripts and move them back to where they were.

Maybe defer could work for the sub menus, but not for the front page.

I am testing like this:

  1. delete old html folder
  2. copy in new one
  3. Launch the game, and see bugs
  4. remove new html/ folder, copy over the old one
  5. launch the game, everything works
  6. go to step 1

I think this simulates a cold boot of the game or something, resetting whatever cache is there.

Testing the latest changes, I am still encountering every single bug I already mentioned above.

I think the only way to move forward here is to remove defer from the scripts and move them back to where they were.

Maybe defer could work for the sub menus, but not for the front page.

I am testing like this:

  1. delete old html folder
  2. copy in new one
  3. Launch the game, and see bugs
  4. remove new html/ folder, copy over the old one
  5. launch the game, everything works
  6. go to step 1

I think this simulates a cold boot of the game or something, resetting whatever cache is there.

Honestly, I still haven't been able to cause such a problem. But if everything works for me, then you don't, which means that some user may also have problems. Therefore, I returned the js binding as it was originally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement The pull request enhances current functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants