1.15.2 Is there a better way to code this?

Discussion in 'Spigot Plugin Development' started by Joiubaxas, Jan 31, 2020.

  1. Hi. I have code to iterate over coordinates based on where the selection is facing to spawn item frames with maps. The firstLocation would always be at bottom left of the selection and secondLocation would always be at the top right. I wrote this code over a year ago and today after reading this I'm unsure if this is the best solution to the problem since the code doesn't seem very elegant.

    Code (Java):
       if (firstLocation.getX() == secondLocation.getX()) {

                if (firstLocation.getZ() < secondLocation.getZ()) {

                   
                    for (double locz = firstLocation.getZ(); locz < secondLocation.getZ() + 1; locz++) {
                        for (double locy = secondLocation.getY(); locy > firstLocation.getY() - 1; locy--) {

                            int splitwidth = Math.abs((int) firstLocation.getZ() - (int) locz) * 128;
                            int splitheight = Math.abs((int) locy - (int) secondLocation.getY()) * 128 ;

                            //spawn item frames and do stuff
                        }
                    }

                } else {
                    for (double locz = firstLocation.getZ(); locz > secondLocation.getZ() - 1; locz--) {
                        for (double locy = secondLocation.getY(); locy > firstLocation.getY() - 1; locy--) {

                            int splitwidth = Math.abs((int) firstLocation.getZ() - (int) locz) * 128;
                            int splitheight = Math.abs((int) locy - (int) secondLocation.getY()) * 128 ;

                            //spawn item frames and do stuff
                        }
                    }

                }
            } else if (firstLocation.getZ() == secondLocation.getZ()) {
                if (firstLocation.getX() < secondLocation.getX()) {

                    for (double locx = firstLocation.getX(); locx < secondLocation.getX() + 1; locx++) {
                        for (double locy = secondLocation.getY(); locy > firstLocation.getY() - 1; locy--) {

                            int splitwidth = Math.abs((int) firstLocation.getX() - (int) locx) * 128;
                            int splitheight = Math.abs((int) locy - (int) secondLocation.getY()) * 128;

                            //spawn item frames and do stuff
                        }
                    }

                } else {

                    for (double locx = firstLocation.getX(); locx > secondLocation.getX() - 1; locx--) {
                        for (double locy = secondLocation.getY(); locy > firstLocation.getY() - 1; locy--) {

                            int splitwidth = Math.abs((int) firstLocation.getX() - (int) locx) * 128;
                            int splitheight = Math.abs((int) locy - (int) secondLocation.getY()) * 128 ;

                            //spawn item frames and do stuff
                        }
                    }

                }
            }
     
  2. We can't really offer a solution if you don't tell us what this is supposed to do.
     
  3. Well I said what it does in the first sentence.
     
  4. So basically he has to iterate over a list of coordinates based on a selection area is facing.. basically the block face.and to put item frames on those blocks in the "selection area" and put maps in those item frames..

    so you need to with a selected area put item frames in a specific order/layout and such and then populate those item frames with their respective map?


    and so if I understand you correctly. the code works however you want to know if theres a more efficient way of doing this.. I would presume?
     
  5. Yes I need to populate the area with item frames that have specific maps. The code works but I don't find it elegant or efficient, so I'm looking for a better way to do this.
     
  6. Maybe like this?
    Code (Java):
    Block block = initialBlock;
    BlockFace direction;
    for(int i = 0; i < width; i++) {
        for(int j = 0; j < height; j++) {
            //spawn item frames and do stuff
            // edit: we need to cancel one step at the last iteration or revert the last step when out of the inner loop
            if (j == height - 1) break;
            start = start.getRelative(i % 2 == 0 ? BlockFace.UP : BlockFace.DOWN);
        }
        block = block.getRelative(direction);
    }
     
    #6 Schottky, Jan 31, 2020
    Last edited: Jan 31, 2020
  7. You can achieve this quite simply, just create a cuboid object and iterate through it for blocks and then update the block faces!
    Code (Java):
    public Cuboid(Location l1, Location l2)
        {
            if(l1.getWorld() != l2.getWorld()) throw new IllegalArgumentException("Locations must be on the same world");
            this.worldName = l1.getWorld().getName();
            this.x1 = Math.min(l1.getBlockX(), l2.getBlockX());
            this.y1 = Math.min(l1.getBlockY(), l2.getBlockY());
            this.z1 = Math.min(l1.getBlockZ(), l2.getBlockZ());
            this.x2 = Math.max(l1.getBlockX(), l2.getBlockX());
            this.y2 = Math.max(l1.getBlockY(), l2.getBlockY());
            this.z2 = Math.max(l1.getBlockZ(), l2.getBlockZ());
        }
    After you create the cuboid you can iterate through it and put each block into a list and then iterate through the list and update the blockfaces.
    Code (Java):

        public List<Block> getBlocks()
        {
            Iterator<Block> blockI = this.iterator();
            List<Block> copy = new ArrayList<Block>();
            while(blockI.hasNext())
                copy.add(blockI.next());

            return copy;
        }
     
  8. How would I get the relative location using this method? In my code you can see that I’m getting relative pixel coordinates (each map has 128* 128 resolution so I’m multiplying by 128)
     
  9. You could use this method to get the relative position to the player(+128 blocks on each axis)
    Code (Java):
    void getRelativeLoc(Player player) {
            Cuboid cube = new Cuboid(player.getLocation(), new Location(player.getLocation().getBlockX() + 128, player.getLocation().getBlockY() + 128, player.getLocation().getBlockZ() + 128));
        }
    The only fault you would have is you would have to be at the lowest point of the selection. But you could add code to make the selection center onto your player.
     
  10. TeamBergerhealer

    Supporter

    One thing I can suggest is to use int rather than double in your iteration logic, so you don't have to do double->int casts inside your loops. This also prevent possible bugs if your start/end location isn't exactly .0 after the block coordinates.

    Another suggestion is to change your loop so you do the subtraction up-front. It works sort of like this (pseudocode):
    Code (Text):
    int dx = second.x - first.x;
    int dz = second.z - first.z;
    for (int x = 0; x < dx; x++) {
        for (int z = 0; z < dz; z++) {
            // Use x/z
       }
    }
    A very clean OOP solution would be to create a Cuboid class, or some class that implements Iterator<Vector> to iterate the blocks inside a cuboid. You could make this as advanced as iterating from p1 to p2, allowing for reverse iteration if specified. Kind of like this class: https://hub.spigotmc.org/javadocs/bukkit/org/bukkit/util/BlockIterator.html