#Data Maps

1224 messages · Page 2 of 2 (latest)

sleek cliff
#

stop doing code recipes

fervent drum
#

Conflicts

frosty token
#

This one begs the question: do we care enough about values higher than max short with vanilla clients to have the furnace patch or do we just say fuck it, it's visual only (AFAICT the result should only be that the flame stays full until the remaining duration falls below max short) and rely on https://github.com/neoforged/NeoForge/pull/599 to fix the short limit on NF<->NF connections?

molten gale
#

vanilla not handling it isn't our problem™️

#

Its the burden of the server owner if they want to make >short max burn times while accepting vanilla clients

#

imo that will literally never happen anyway

frosty token
#

In the end, the info shown to a vanilla user for burn times higher than max short isn't necessarily wrong, the flame just stays full until the remaining time drops below max short

#

(Though, please test that assumption)

sleek cliff
#

the litduration is also a short

frosty token
#

Yes, so you have two values stuck at max short, whose division will result in 1 and therefore a full flame (purely from a logical code analysis). Once the remaining time falls below max short, the flame starts dropping as expected again.

hybrid wedge
#

Depends how you implement it

#

By default you might get garbage flame results

analog olive
#

if we cap to max short that's one thing, but aiui the vanilla code just does a bit truncation

tacit folio
#

@frosty token did you test the part data map or is it just a code review and I should test it before merging it later

frosty token
tacit folio
#

Okay

tacit folio
#

well time for me to move mek additions over to it and test and then merge it

#

once I muted hostile mob sounds so I could try and listen for the imitate sounds without being hard for me to tell what was real vs what was an imitation it seems to work well. shipit

hybrid wedge
#

how do data maps handle vanilla server and neoforge client again?

sleek cliff
#

it doesn't. these values we have been replacing are all server sided

#

or rather, used by vanilla only on the server

#

they're optionally synced for other mods like recipe viewers or whatever

hybrid wedge
#

what about the FurnaceFuelSlot used by the furnace menu?

sleek cliff
#

I don't think the client checks any fuels, when I tested the short stuff I used a vanilla client with a neo server that added 38k to anvils

tacit folio
#

maty, tech means neo client on vanilla server

#

because if the backing map is empty for what is considered a fuel

#

then it may not let you place it in there

sleek cliff
#

re read what I said Stabby

sleek plover
#

Is there a TODO for converting data maps

sleek cliff
#

if a vanilla client doesn't check it, neither will a neo client

tacit folio
#

isFuel is net.neoforged.neoforge.common.CommonHooks.getBurnTime(p_38989_, this.recipeType) > 0;

Which means tech is likely right that if the datamap isn't read (due to the VANILLA_BURNS no longer being a thing) then it will probably return false for that when neo client on vanilla server

frosty token
tacit folio
#

I suppose, though it also isn't that bad a patch to make it support it correctly instead of being just partially wrong

sleek cliff
#

tis the 5th time i test neo&vanilla, i'm getting tired of retesting my tests Stabby

hybrid wedge
#

even if you shift click?

#

what is the point of having these functions if vanilla doesn't even check them tired

sleek cliff
#

yes

#

even if i shift click

sleek cliff
#

since like, i still can't shift click a random item nor put it there manually

hybrid wedge
tacit folio
#

may place looks to be used by the container screen for dragging an item into multiple slots

#

and that is the only screen based usage I see

#

granted why you would be dragging to split between stacks with a furance, especially when one of the slots you want to target is the fuel slot... I have no idea

sleek cliff
#

so half assed

#

yep, everything else is handled by doClick/quickMoveStack on the server

#

this is the only use in vanilla that would be on the client

image.png
tacit folio
#

personally I don't think it is necessarily worth the effort to handle that one case for neo client connected to a vanilla server as I have no idea why someone would ever use that functionality for a furnace... Though I definitely think it is worth a note somewhere about that specific scenario

#

though inventory sorting mods might be using it as well

#

so if they are client side only then who knows how it would work with furnaces

sleek cliff
#

i mean until today i didn't even realise you can use dragging in furnace slots

#

lol

#

but a sorting mod shouldn't put fuels into the furnace when sorting, that's bad™️

#

it's randomly burning user resources

tacit folio
#

you know what I mean

#

in regards to that type of client side mod

sleek cliff
#

1+1=2?

tacit folio
#

whether one may care

#

I don't know why they would but kaishrug

#

but yeah I don't really care too much as long as you say put a note in the javadoc for the burn data map

sleek cliff
#

also since when are full slots yellow after dragging

image.png
tacit folio
#

how @hybrid wedge feels about it idk though

#

at least 1.16.5

hybrid wedge
#

idk, seems like we might have problems with other potential data maps?

sleek cliff
#

fallback data maps? australian_tater

#

loading the fallback from the mod jar is an option ig

hybrid wedge
#

same problem as fallback tags I would say

sleek cliff
#

which problem

#

with a fallback tag possible desyncs would be extremely obvious and annoying, but in this case a desync would affect one particular obscure feature

#

(that is, dragging fuel into a furnace slot)

#

i dunno h ow to handle this, but i think this corner case is very stupid and doesn't matter

#

🤷‍♂️

tacit folio
#

I don't think it matters beyond making a note about in the javadocs for the datamap type. And other than that say introducing fallback datamaps is semi out of scope of this particular PR

hybrid wedge
#

I don't really care as long as we have decent vanilla client or server compat

tacit folio
#

yeah it is such an obscure usage that breaks I don't think anyone will actually notice the difference

#

so maty, amend the javadoc to note about the corner case that is known to not work when connecting to a vanilla client, and fix merge conflicts and I will approve and merge it

sleek cliff
tacit folio
#

shipped

analog olive
#

server syncs back, but it won't undo if the client thinks an item got quickmoved into a slot that didn't get touched on the server

#

it mainly becomes a problem in this case if someone wants to disable an item that is valid vanilla fuel, though.

#

[er, for vanilla client / neo server, anyway]

sleek cliff
#

I mean we'll support vanilla on the other side to a limited extent usually

sleek cliff
sleek plover
#

@sleek cliff I can't seem to access the builtinRegistryHolder for VillagerProfession. mind helping?

whole wren
#

Ask in #modder-1․20․1-support

sleek plover
frosty token
#

@sleek cliff do you want me to hit you with the typos on the docs PR? 😛

sleek cliff
#

yes

sleek plover
# sleek cliff yes

I can't seem to access the builtinRegistryHolder for VillagerProfession. mind helping?
mind helping me with this (if you have time ofc)

sleek cliff
# 
{
  "values": {
    "minecraft:acacia_fence_gate": {
      "axe_strip": {
        "type": "idd:combined",
        "modifications": [
          {
            "type": "idd:state_matches",
            "predicate": {
              "open": "true"
            },
            "transformation": {
              "type": "idd:constant",
              "value": {
                "Name": "minecraft:iron_block"
              }
            }
          },
          {
            "type": "idd:constant",
            "value": {
              "Name": "minecraft:dirt"
            }
          }
        ]
      },
      "till": {
        "type": "idd:constant",
        "value": {
          "Name": "minecraft:birch_fence_gate",
          "Properties": {
            "facing": "north",
            "in_wall": "false",
            "open": "true",
            "powered": "false"
          }
        }
      }
    }
  }
}
#

mhh

#

some experimenting with data maps in one of my mods

hallow wing
#

why are we returning 0 here instead of -1?

hybrid wedge
#

previous code was ret == -1 ? VANILLA_BURNS.getOrDefault(item, 0) : ret

ripe lion
#

Because that's correct?
0 is for unburnable

#

We use -1 as an intermediary value from our extension to fall back to the vanilla handling

hybrid wedge
#

0 is correct, -1 is only the return value from IItemExtension#getBurnTime to indicate that it should defer to vanilla logic

#

what ater said

hallow wing
#

right..., except now you get two different values for the exact same functionality depending on which method you're needing to use to retrieve burn time

hybrid wedge
#

you should never use the itemstack method

hallow wing
#

then deprecate it for removal

hybrid wedge
#

yeah it is pretty stupid that there is a CommonHooks method when it could just be in the itemstack method I suppose

hallow wing
#

idk man

#

all I'm saying is

#

we now have two access points

#

which return two different values for the same thing

#

that's not great from an API standpoint

hybrid wedge
#

even worse, the itemstack method will not call the event and is therefore incorrect

ripe lion
#

? This is the same as any of many of our other hooks declared on the extension, some of them are their to be overridden, not called.

hybrid wedge
#

the Item method should be overridden

#

but since ItemStack is final there is no reason not to put all the logic there

hallow wing
#

^

#

if I didn't coincidentally stumble on the commonhooks method while looking at the datamap, I wouldn't have ever known it existed

#

and would've used the itemstack method

ripe lion
sleek plover
compact sorrel
#

What is this again

#

Ive been sleeping a while

sleek cliff
#

#neoforge message datamap, anyone? /s

civic ledgeBOT
hallow wing
#

Last I saw orion was blocking it because they know the use case of modders better than the modders themselves who are requesting it

#

Forge flashbacks anyone

molten gale
#

Well the good thing is we don't need orion's approvals for things :p

hallow wing
#

Funny you say that

#

Because its seemingly his feedback that prevented that pr from gaining any traction

molten gale
#

is there a pr for it?

hallow wing
#

Like I’d waste my time doing a pr when I know that’s the nonsense I’d get

molten gale
#

The rest of us have the authority to merge, and imo you have mine and tech's approval there

#

I would prefer it be controlled enough (like more sophisticated than just a public mutable set)

hallow wing
#

I don’t disagree

#

A datamap might actually be a good target

#

Maybe

#

Actually type safety would probably pose an issue there

molten gale
#

Plus you have to handle merging

hallow wing
#

Yeah

molten gale
#

which tends to be a pita

hallow wing
#

Might be better as either a hook adder or an event

#

Preference?

sleek cliff
#

data maps can handle merging

#

but not sure if a data map that can be configured by users is a good idea harold

hallow wing
#

Yeah not for this one

molten gale
#

Generally speaking we have been pushing for event based registration of auxiliary data

hallow wing
#

The main issue with an event here is

molten gale
#

But thinking about that event name sounds horrible lmao

#

RegisterExtraBlockEntityBlocksEvent kekW

hallow wing
#

Itd also be a completely unnecessary additional split of registration functionality

sleek cliff
molten gale
#

Like all the client maps

hallow wing
#

You’re doing this to register your block, then you’d have to either store that in a map/list or manually write out each one to send to the event at event time

#

As opposed to just method calling the adder when you register your block or whatevrr

molten gale
#

The BEType's owner wouldn't go through this process

hallow wing
#

Yeah that’s what I’m referring to

sleek cliff
molten gale
#

Its probably a trivial amount of boilerplate

#

Not like someone is going to be adding that many blocks to external BE's

hallow wing
#

signs are pretty popular tbf

hallow wing
molten gale
#

Just make it a static method on BE that idk, locks after registration phase or smth

fervent drum
analog olive
#

i've been pushing for an event because it's an immutable map and even if we make it mutable cross-loader mods may replace it with an immutable map

#

er, set

#

and the fact that it is an immutable set means that allowing multiple mods to add to the same one would benefit from a gather phase rather than having each operation copy the set

hallow wing
#

Event is consistent with the existing stuff too so yeah

molten gale
#

but that event name will just be so bad lmao

analog olive
#

BlockEntityTypeValidBlocksEvent

molten gale
fervent drum
analog olive
#

BlockEntityTypeEvent.ValidBlocks [do we have other BlockEntityTypeEvents? not yet]

#

i disagree

molten gale
#

BlockEntityType#registerAdditionalBlock as an instance method should be fine™️

distant plaza
#

DataMapsEvent with generics /jk

fervent drum
molten gale
#

it invites nothing more than the event

fervent drum
#

For one it leaves open the question of timing

#

Parallel register or nof

analog olive
#

RegisterBlockEntityTypeValidBlocksEvent

fervent drum
#

An event is much more structured and in line with systems

analog olive
#

WaxedLightlyWeatheredCutCopperStairsEvent

molten gale
#

it only leaves open whatever questions you decide to leave open

fervent drum
#

Yes so?

#

We are moving away from arbitrary registration methods and maps. Precisely because they invite threading issues and problems with registration timings. I see no reason to change that here.

molten gale
#

they only invite threading issues because for some reason people were allergic to making synchronized registration methods

fervent drum
#

Yes. Because they put performance of boot up time in the bin

analog olive
#

ok so what is the timing

#

if we can say "this is only valid during commonsetup" surely we can say "this is only valid during the enqueued work phase of commonsetup"

molten gale
#

Was that boot time thing ever a concern? Pretty sure model load and voxel shape init still dominate boot time

#

like so much so that this is a non-issue

#

the problem was people just not using enqueue

vagrant cliff
#

is there a way to specify an item as a recipe ingredient ? only allow items that have a specific piece of data attached to it

#

or should I use them in combination with tags

hybrid wedge
#

are you talking about data maps or data attachments?

vagrant cliff
#

datamaps

sharp bone
#

you'd need a custom ingredient type for your specific check

#

I think

hybrid wedge
#

You should probably use a tag for recipe inputs

vagrant cliff
#

the docs mention "map-based datamaps" and "collection-based datamaps". is there an example for those ?

sleek plover
#

collection based?

hybrid wedge
#

look at the advanced data map type

sleek cliff
#

check the tests mods too

vagrant cliff
#

tests are great examples!

#

datamaps are applied to registryholders right ? so in cases of Items, they will only work with specific Items or would they also work with Tags ?

#

I want to apply some default value to a Tag which then I can override per Item (which has the tag)

# 
        .add(ItemTags.LOGS, new SomeObject(156, "some other string"), false)

it does work! Great!

vagrant cliff
#

@sleek cliff can I DM you regarding a certain implementation that I want to make using Datamaps ?

hybrid wedge
#

No

#

Ask in one of the development channels 😛

vagrant cliff
#

I would have but I dont think that is gonna work. It needs a bit of info dump from my side to explain what I want to do but that might just get lost in chat

#

I will still put a message there just incase #modder-1․20․1-support message

vagrant cliff
#

How expensive are getData() calls ? does it have any innate caching system ?

sharp bone
#

getData is a map lookup

#

O(1) on the general case

#

O(log n) worst case (the worst case is unlikely to happen)

sharp bone
vagrant cliff
#

okay

sleek cliff
#

it's 2 identity map lookups