Carpet

Carpet

2M Downloads

Few issues with Scarpet draw and distance apps

altrisi opened this issue · 13 comments

commented

List of issues (detailed below):

  • [Distance] Lines and labels are too near to the ground
  • [Distance] Right clicks block functionality was changed again after merging
  • [Draw] Update handling may be overcomplicated
  • [Distance] If lines are enabled, it will draw infinite lines in the same position Not Applicable anymore
[Distance] Lines and labels are too near the ground

When using the line functionality in the distance app (mostly with the brown carpet), the label and the line gets inside of the carpet, and it's directly on the floor. This could be solved by drawing the line slightly higher

[Distance] Right clicks block was changed back to places block after merging

The right_clicks_block event was used instead of the places_block due to parity with the Java command. The Java command allows to set positions with the carpet even when the carpet doesn't actually get placed (like right clicking in walls), and that's the reason why it was changed (see #553 (comment)). It was "reverted" in 4c6d449

[Draw] Update handling may be overcomplicated

This was readded right before merging, in 623f9f3. As already commented in the pull request (and it was removed at that time), updates are already sent from Scarpet's set function when fillUpdates is true. However, it was added back last minute without a second to comment, overcomplicating both affected() and set_block, due to adding each pos into a list instead of just adding +1.

The change needed to revert this would basically be about removing the without_updates wrap in set_block, only adding +1 to global_affected (maybe even remove the if block != block since that is already handled by Carpet and do global_affected += set(...) since booleans are numbers, or similar), changing global_affected to be and be reset to a number again and printing just the variable.

[Distance] If lines are enabled, it will draw infinite lines in the same position

Tired of writing, but this is an important one. See the thing and a sketch for a workaround in #553 (comment) (slightly outdated). "Part of the fix" was kind-of added, but the most important part wasn't (not scheduling a new line)

Other small things that may or may not be considerable

  • [Distance] The label has almost no information (probably still WIP)
  • [Distance] The label recalculates the distance every time it's rendered
  • [Distance] It's not necessary to check for start when drawing since it will always be present when end is
commented

regarding 1) yes they need to be little higher to be practical to use.
regarding 2) yes and no - since it was in the carpet block place code, it was in a very peculiar place. Right clicking on a wall worked (no support below), but on the other hand, rightclicking in a space already occupied by another block didn't - that's weird. These apps are meant to be same or better, not same. Original intention was to provide measure when blocks were placed (even if they popped right off, like in water). I don't see a problem with that.
3) yes - don't know if that was to mimic way fill handles blocks, by updating them right after, or what the reason is for that - maybe there is, but see no problems setting them normally in this case.
4) yeah, that part can be handled a bit better. I think each line should be handled on its own, and there should be a 'clear command / behaviour to clear them. Not sure what at this moment.
re smaller things:

  • little information is good if its adequate information
  • its a simple operation - not sure why this would be a problem
  • doesn't harm
commented

I want to have more overlay

commented

k Ill fix it

commented

Well, I already added the replacement functionality - just update your branch

commented

I think the reason for this kind of updating is cases like filling in water blocks inside lava area or vice versa. Immediate updates when setting the blocks means that you will get a checkerboard pattern, which I think you are actually getting in vanilla - these cases can be tested.

commented

also - I removed cross dependency on rules between each other. In distance - it now has its own part in the command to define the block assister.

commented

Regarding 3): I did it cos that way I didn't have to query fillUpdates every time, and yes, cos DrawCommand.java does that. idk if I should always update. and I hadn't actually added replacement implementation, so imma add that, after I figure out the block predicate thing.

commented

I saw dat. im owrking on the block caching rn.

commented

Suggestions:

How bad would it be to keep brown carpet as a default assist? It's the current default behaviour, and the assist is already dependant in the carpets rule, which is off by default (btw if that is intended the events should probably only be enabled when that is on, and it could be hooked to an on_carpet_rule_changes event (would have to be made available for player scope, didn't really think when making that event) to toggle it).

And maybe allow assist to be used without carpets enabled when it's set to something else than a carpet, or something similar.

Also maybe keep chat messages even with shapes in case of clients that can't render labels. Too much sss

commented

I have already added control over assist block to the command - apps should be self-contained, and not dependent on each other - for clarity - currently it suggests brown_carpet, or none, but accepts any block. I also added shift_use detection to toggle display modes and use (without sneaking) to undo last measure.

Suggestion for better support for non-carpet clients is valid - they can still use shapes, but labels would be send only as chat messages. For that you would need to add something to the entity API allowing to distinguish that (like e ~'carpet_client').

commented

@gnembon r u gonna do this or shall I?

commented

cos Ive alredy done some things, and seen u alredy do them, soo...

commented

k can I do the rest? Ive already done the stuff on a branch, but kept it private incase u were gonna do it