[request for feedback] minor everest annoyances

[glacial spruce]

resurrecting

[glacial spruce]

(link to scroll to top)

[earnest thunder]

🕯️🕯️

[glacial spruce]

here are a number of issues i don't feel are ready for full issues yet

• would be good to be able to detect newly created files (eg. English.txts) and load them at runtime
• would be good to autodetect doubled extensions English.txt.txt heuristically + warn somehow
  • where should this be done?
• make the copy tileset attribute not do that
  • #993155184815517789 message
• collision (& rendering when safe to do so?) optimisation
  • would quadtree, k-d tree, or bvh tree be better?
  • check with tas people as to if this could be safely applied to vanilla / popular maps
• review all MonoModRules rules to use use ILCursors where appropriate; better document what they do & why
• multiline minitextboxes
• decal scale/rotation optionally applying to hitboxes, particles etc.?
• apparently the core spinner juice changes colour one frame after the spinner itself
• metadata option for the background colour that's rendered before stylegrounds

[alpine olive]

Double extensions might also be a good option for Max's mod structure verifier

The decalregistry addition can definitely be an opened issue

[earnest thunder]

going to second the decalregistry option for decal scaling, it’s subtle but really important for clouds :>

[glacial spruce]

personally i find the structure verifier is pretty late in a lot of workflows (eg. as a final check prior to upload), so while it definitely should do this i feel it shouldn't be the first line of defense
i favour putting it in lönn as a first line of defense because that's where people will be setting dialogue keys (i'm ignoring ahorn)
imo putting it in log.txt isn't very helpful as it'll get buried & not read

[proud lintel]

i don't see decal rotation options in the registry being of much use, there are very simple ways to rotate an image in built-in image editors (on windows and mac at least from personal experience) if you want it to apply to every decal with that path, individual rotations sound more useful

(decal registry applies the effects for every decal with that corresponding path)

[alpine olive]

I think the rotation is supposed to be built in, not part of the registry

[glacial spruce]

lol can't believe i forgot the odd-sized decal jank; put that in the list
(i looked at the code and i'm pretty sure it's because of drawcentered, so not due to multiple of 8)

[glacial spruce]

supposed to be? can you elaborate?

[alpine olive]

Sorry, I mean I assumed that's what the proposal is for

[glacial spruce]

ah i edited it. i was proposing both originally

[alpine olive]

Gotcha 👍

[mental torrent]

Usually Lönn doesn't care at all about the English.txt file

I'm not sure how it could be inserted into Everest though, so I don't have a better idea

[glacial spruce]

yea but it's where you mostly put in dialog keys, so it makes sense to validate stuff there
might be nice to have an indicator for if the key is valid next to the field or something

i mean it could be a postcard, but that seems overly obtrusive for what's effectively a warning

[glacial spruce]

idea for SamplerState.PointWraped backdrops

(doesn't work properly; displays the whole atlas for some reason)

not going to implement this one myself unless i can figure out how virtualtextures work

[hybrid kiln]

collision (& rendering when safe to do so?) optimisation
imho if we were to do this, i would go with a BVH (bounding volume hiearchy) - they're self balancing binary trees (e.g. when an object moves they can efficiently maintain their balancing), balanced using surface area, which would allow for a very fast collision broad phase. i'm not sure but we could potentially also use them to cull out all objects which don't intersect with the camera rectangle
as for further optimizations, we might want to rewrite some narrow phase algorithms (like e.g. object-tile, ray-tile, object-sphere etc.) as well, in such a way that they take advantage of the new datatstructure (we should be extremly carefull with this though, and maybe only enable it for maps which specifically want them to using e.g. a meta flag, as we could potentially desync tases which take advantage of slight rounding errors in the algorithms)
we could also then remove the requirement that all CollideWith<> targets are tracked, as the BVH would take care of the performance bottleneck which would have been caused by this

[glacial spruce]

yea a meta flag sounds good

removing that requirement would be really nice for modding

[hybrid kiln]

that would be specific to the near phase though, we could very likely enable the broad phase BVH optimization for all maps (it only changes which objects are considered for collision, so assuming we don't have any bugs there the actual behaviour won't change)

i might take a shot at coding this (i've coded dynamic online BVHs before already) after i finish procedurline / some other projects which have been stuck in my pipeline for uhm..., a long time?

[glacial spruce]

have to be careful culling objects for rendering, because some entities might be visually much bigger than their hitboxes (eg. controllers); making it opt-in would solve that

[hybrid kiln]

i would probably just optimize only sprites, because i can look at their texture to find the biggest bounding box they could render in

only problem is that the visual bounding box might fluctuate a lot though

[glacial spruce]

yea but be careful for if they do PointWrap stuff

imo it's best left opt-in and applied to vanilla entities

[hybrid kiln]

idk if object culling would even be that big of an improvment

we might also encounter side effects from objects doing stuff in Render that they shouldn't do

or mods hooking vanilla render methods in a way that the bounding box doesn't match anymore

[glacial spruce]

it might be for rooms with many many spinners

[hybrid kiln]

my main worry is 1. hooks changing render behaviour 2. visual bounding boxes fluctuating every frame causing unnecessary BVH rebalancing

2. might be fixed by only allowing the visual bounding box to grow

and maybe, just maybe we can hook into low level monocle / fna rendering functions to calculate the bounding box very, very low level, so that we can always correctly determine it based directly on the draw calls

[glacial spruce]

sounds kinda jank tbh; i'd much prefer the entity having to pick its visual bounding box; in fact even hooking fna might not be enough because there's stuff like my windowpane helper windows, which select a leader to do all their rendering (so if its beforerender isn't called, the others try to draw from a blank render target)

[hybrid kiln]

yeah, also collision optimization should be our first goal anyway

[glacial spruce]

new backdrop thing. i lied about not implementing this myself

[wintry birch]

too bad most of the performance issues in the game stem from weird everest things (such as moon blocks in 9d's last room) or very unoptimised modded entities/backdrops, not from collision

as for entity culling, the "performinator" from pandora's box does that (although it can break with entities with nodes and such), and it's used in all lobbies because of how much it can improve performance for free

[glacial spruce]

if that moon block thing is an everest thing it kinda should be on the list

[hybrid kiln]

so having something like that integrated into everest would be a good thing?

[glacial spruce]

and a bvh tree system would provide a free performance boost (collision optimisation) to modded entities + another one with only a little effort (culling)

[hybrid kiln]

as said earlier, i might give coding this a shot once i'm done with procedurline

[glacial spruce]

maybe we could have an experimental 'more culling' setting to cull more dangerously

[glacial spruce]

sounds good! imo this is a pretty big change and should be given a bit of time to think over before implementation, so probably a good thing i can't jump into it now (i don't know how bvh works)

[wintry birch]

each solid iterates through each room of the map each frame, it just so happened that moon blocks exposed this issue in 9d

[glacial spruce]

...why do they do that?

[wintry birch]

to get the current room's bounds

[hybrid kiln]

[wintry birch]

because of course that iterates all rooms :p

[hybrid kiln]

that... should be fixed? definetly?

[wintry birch]

it's not everest's fault per-say, but it can be fixed in everest

as you can see, collision is nothing compared to this simple bad implementation

[glacial spruce]

surely that could be cached in Level

[hybrid kiln]

no react perms

so no

[glacial spruce]

there you go

[hybrid kiln]

[wintry birch]

at first when I saw that function call in the profiler I assumed it was an everest patch to not do some hackfixing in vanilla, but turns out it's just Level.Bounds being horrible

gotta love properties hiding expensive calculations :)

well expensive only in extreme situations like 9d but yknow

[glacial spruce]

imo it's not extreme, it's a popular map; farlands behaviour is extreme

[wintry birch]

well "extreme" in terms of room count

most maps don't have 400+ rooms

[glacial spruce]

until 9<unprintable character> releases with 100000000 rooms going out to x=float infinity, high room count perf issues are everest's concern

[glacial spruce]

yea true. but not outside of 'expected' imo

[wintry birch]

alternatively MapData.Get could use a Dictionary for this, depending on how often this is used outside of getting level bounds

[glacial spruce]

might as well, if it's not hard to patch and mods aren't patching it, there's no harm

[wintry birch]

oh I see, it's called for each entity created too :p

alternative 3: session.LevelData could be cached

as that's pretty much the only time MapData.Get is called

[glacial spruce]

i'll make an everest issue :)

[wintry birch]

[glacial spruce]

i picked this option; can you verify that my approach (https://github.com/EverestAPI/Everest/pull/466) actually improves performance?

[wintry birch]

trying it now

yup, floaty blocks are now substantially faster

[wintry birch]

@glacial spruce !!!! this pr breaks map reloading - entity changes don't get applied after saving a map

[hybrid kiln]

probably have to clear the leveldata cache after reloading the map

[glacial spruce]

oops

[limber cave]

time to switch to plan b

alternatively MapData.Get could use a Dictionary for this, depending on how often this is used outside of getting level bounds

[glacial spruce]

yea that's what i'm thinking

[wintry birch]

well tbf the dictionary would probably have similar caching issues

[glacial spruce]

maybe wrap the list in some way and detect edits?

[wintry birch]

well you'd still need to worry about code mods potentially breaking if you change too much

[wintry birch]

this seems quite reasonable still, iirc the LevelLoader ctor would be a good place to clear the cache, haven't tested though

[limber cave]

why, I think we create the dict when mapData.Load()

[glacial spruce]

checking if this works

[wintry birch]

well, only one way to see

[glacial spruce]

uh

System.InvalidOperationException: There was an error reflecting type 'Celeste.SaveData'. ---> System.InvalidOperationException: There was an error reflecting field 'CurrentSession'. ---> System.InvalidOperationException: There was an error reflecting type 'Celeste.Session'. ---> System.InvalidOperationException: Cannot serialize member 'Celeste.Session.LevelData' of type 'Celeste.LevelData', see inner exception for more details. ---> System.InvalidOperationException: Celeste.LevelData cannot be serialized because it does not have a parameterless constructor.

(got this in glyph just before teleport to first checkpoint)

i haven't changed patch_leveldata

[limber cave]

no, reloadmap endpoint reload the mapdata but not reload the level

[glacial spruce]

so on mapdata.load then?

[wintry birch]

well that's not good, make sure Session.LevelData is not public or it will be serialized

or actually, add an [XmlIgnore] to it

[glacial spruce]

nvm i had an internal field to invalidate it; replaced it with a private method

...can i even get the session from inside mapdata.load?

ok i've come up with a janky solution i think

[glacial spruce]

just checking, invalidating the cache in MapData.Load() covers everything right?
my solution (checked with editing a map and letting auto reload do its thing) is to have a number in the MapData instance and a number in the Session instance and to invalidate the cache if they're different; the MapData number goes up if Load is called

[hybrid kiln]

that would break after 2^32 consecutive loads without using the data

smh unacceptable bug

[limber cave]

I think plan b is more cleaner, just recreate the dict every time mapData.Load() is invoked

[glacial spruce]

oh yea that's true
could do both i guess

[hybrid kiln]

i smell potential memory leaks if the dict is not tied to the level lifetime

ah sorry misunderstood

[glacial spruce]

@limber cave @hybrid kiln ok i've added this to the pr and it seems to work, does my implementation look ok?

[hybrid kiln]

sadly can't take a look rn, am on my way home

[limber cave]

@glacial spruce just found game crash when entering Randomizer map
it did not call the load method when generating the mapData, so levelsByName is null
it seems we need to check in the mapData.Get whether levelsByName is initialized and whether the number is same as levels

[glacial spruce]

is it possible for the Levels to be replaced but the count to stay the same?

[limber cave]

yes, but do any mods currently do that?

[glacial spruce]

don't think so unless there's an edge case in rando. i'll make the cache regen method public so if problems arise they can just call that

[glacial spruce]

@frozen spire https://github.com/EverestAPI/Everest/pull/472

[nocturne cloud]

maybe test randomizer endless mode to be safe, that seems like a possible edge case

[limber cave]

it is safe since it will generate another map after collecting heart