Postby Emral » Sun Oct 23, 2022 10:43 pm
Gave it a read. There's a lot that can be improved:
- Adding new elements to the _G table is almost never a good idea, even less so when using table names that are intentional typos of basegame table names like "Playur". I highly recommend to instead just have the sublibraries be loaded as fields in the smasfunctions table, so that users have control over whether or not to make things local or global scope by placing or omitting the "local" ahead of assigning the smasfunctions variable when loading it.
- It is worth noting that anyone following the code's recommendation of using the SEE mod will make their episode incompatible with SMBX2. Proceed at your own risk.
- camera.lua:
-- Screen.x/y/width/height functions form a tuple that is often called back-to-back to get each individual component. In practice this means that when using customcamera you are calculating the bounds up to 4 times when you only need to do it once. I think a function that returns all 4 properties as a vector or as separate return arguments would be more appropriate than having them split into 4.
-- Same for cursorX and cursorY, they just seem neater to compress into a vector immediately
-- The onscreen check doesn't support default camera
-- The oldBoundary___-variables are never used. What are they for?
- event.lua:
-- You are attempting to register onDraw when it doesn't exist
- img.lua:
-- The path names here are strange. Why would you store images in the scripts folder? The _OST folder? loadImageResolved also already looks in the graphics folder.
-- Img.draw, while claiming to be better than the stock options for drawing, is terrible for performance. The reason being that it loads the image every time it is executed. Unlike default drawImage-variations, there is also no support for spritesheet animation, and unlike using the sprite class there is no support for scaling, rotation and shaders. I don't recommend anyone use this function over the alternatives.
- Misc.lua
-- Misc.episodeFilename is a misnomer, as it returns the filename of the world file, not the episode.
-- Misc.unlockAnyBrokenPaths is unusable, as well as woefully inefficient.
-- Misc.checkTargetStatus is a misnomer, and does not offer any value over the customCamera.targets field
-- Misc.saveSaveSlot's input vetting is incomplete, allowing save slot -7548134 to be saved to.
-- What's Misc.overrideLibrary for? I can only imagine griefing.
- NPC
-- NPC.harmAll claims to harm all NPCs with a harm type but only harms the passed NPC. It also uses vararg for seemingly no reason
-- NPC.harmSpecified doesn't work as advertised because it returns after the first harm
-- Whatever saveClass does, it doesn't look like it will work. The data structures don't line up. I also don't understand why the NPC class extensions have something that seems to be like savestates for BGOs and layers included.
- Player
-- Playur.countEveryPlayer has a useless outer for loop
-- Why does Playur.hasCharacter not just return true/false?
-- startPointCoordinateX/Y could be combined to return a vector
- Sound
-- Sound.changeMusic and its sister functions should be using Audio.MusicChange
All in all, I wouldn't advertise this as being "for easy coding", as the topic title suggests. Aside from being highly specific, most functions here that are generally useful are simple wrappers around existing functions. For easier use in specialized cases, removing things that wouldn't work for other users (SMAS++-specific functions) to reduce confusion would help. Documentation in-code above the functions would also make things more organized.